12:37 p.m.
On Fri, Nov 11, 2022 at 01:17:01PM +0100, Laszlo Ersek wrote:
On 11/10/22 00:26, Eric Blake wrote:
> Improve some existing tests to cover client safety valves for oversize
> requests, and add a new test that tests libnbd behavior on server
> errors to the same sort of requests. The new test requires a parallel
> patch posted to nbdkit to teach 'nbdkit eval' how to forcefully
> disconnect the server on particarly oversize write requests, emulating
s/particarly/partially/ (I believe)
Actually, I meant 'particularly'. Either way, though, it was indeed
an unintended typo; thanks for spotting it.
> + /* Disable client-side safety check */
> + strict = nbd_get_strict_mode (nbd) & ~LIBNBD_STRICT_PAYLOAD;
The current macros are (without the first patch applied):
#define LIBNBD_STRICT_COMMANDS 0x01
#define LIBNBD_STRICT_FLAGS 0x02
#define LIBNBD_STRICT_BOUNDS 0x04
#define LIBNBD_STRICT_ZERO_SIZE 0x08
#define LIBNBD_STRICT_ALIGN 0x10
#define LIBNBD_STRICT_MASK 0x1f
The 0x prefix *permits* the compiler to consider unsigned integer types
when picking the size (type) for the integer constant, but the prefix
does not *force* an unsigned type. And, because these values are really
small, they all have type "int". In turn, when we apply the bitwise
negation operator, we flip the sign bit. Not necessarily a problem on
the platforms we care about, but not nice.
There are two ways to remove this concern. One is the following:
strict = nbd_get_strict_mode (nbd) & ~(sometype)LIBNBD_STRICT_PAYLOAD;
where "sometype" is an unsigned integer type having at least the
conversion rank of "int" (so that it would not be promoted to
"int"),
*and* large enough to accommodate all uint32_t values. C99 permits
"unsigned int" to be just 16 bits, so using "unsigned int" for
"sometype" is only safe if we are sure that on our platforms, "unsigned
int" will always be able to represent "uint32_t". Otherwise we need a
larger type, such as "unsigned long" (which is guaranteed by C99 even to
hold a uint32_t).
And as the addition mirrors similar other spots in the code base, we'd
have to edit those as well.
The other way is more generic: change the generator to output all these
constants with a "u" suffix, such as:
#define LIBNBD_STRICT_COMMANDS 0x01u
#define LIBNBD_STRICT_FLAGS 0x02u
#define LIBNBD_STRICT_BOUNDS 0x04u
#define LIBNBD_STRICT_ZERO_SIZE 0x08u
#define LIBNBD_STRICT_ALIGN 0x10u
#define LIBNBD_STRICT_MASK 0x1fu
This is better because the compiler will pick "unsigned int" for them
(once we reach huge values, the compiler will pick "unsigned long" then
"unsigned long long"), the bitwise negation will be applied to unsigned
integers, and then the bitwise "and" will be between a uint32_t and some
unsigned integer type (and at that one that's not going to be promoted
to "int", having a rank already at least as large).
Yep, that was my first thought as well as soon as I started reading
your note about the corner-case C definedness of the operation.
Having all our flag values be generated as unsigned from the get-go
makes total sense, so I'll do that as an additional patch.
Then in case "uint32_t" fits in an "int", it's going to be
promoted to
"int", and then the bit-and is between int and unsigned int --> the int
is converted to unsigned int, done. Otherwise, we have two unsigned
integers, the one with the smaller rank is converted to the type of the
other.
Long story short: probably totally harmless in practice on our platforms
(the code happens to do what's expected due to two's complement
representation of signed integers), but it's general hygiene to avoid
bit manipulations on signed integers (some of those happen to be
undefined behavior even). It's good to be aware IMO that the generator
currently produces LIBNBD_STRICT_* values that have type "int", and not
signed int.
Indeed - totally harmless on all platforms we care to support, at
least until some new clang sanitizer starts griping at us, so we might
as well improve our definedness.
There's a very slight chance that someone could complain that flipping
our definition from signed int to unsigned int constants is an API
change, but I don't see anyone writing code that intentionally tries
to care about the signedness of the bits. It is NOT an ABI change, at
least on the platforms that we compile on.
Feel free to ignore this comment if you think it's needlessly cautious.
(I think it'd be best to keep the patch as-is, and then modify the
generator in a separate patch.)
Absolutely a separate patch; already posted.
...We could also consider the C99 standard macro UINT32_C(...):
#define LIBNBD_STRICT_COMMANDS UINT32_C(0x01)
#define LIBNBD_STRICT_FLAGS UINT32_C(0x02)
#define LIBNBD_STRICT_BOUNDS UINT32_C(0x04)
#define LIBNBD_STRICT_ZERO_SIZE UINT32_C(0x08)
#define LIBNBD_STRICT_ALIGN UINT32_C(0x10)
#define LIBNBD_STRICT_MASK UINT32_C(0x1f)
which creates values of uint_least32_t -- but that type can (in theory)
still be promoted to "int", if "int" is large enough. So I think the
"u"
suffix is best.
My recollection (without digging out the C99, C11, or C2x standard at
the moment) is that C allows:
char: at least 8 bits (a 9-bit char is viable on some hardware, as
well as 32-bit char on others)
short: at least 16 bits, but at least sizeof(char)
int: at least 16 bits, but at least sizeof(short) (16 and 32 are the
most common, but I think I've seen documentation for various
hardware with 18- or 24-bit ints)
long: at least 32 bits, but at least sizeof(int)
long long: at least 64 bits, optional
signed values have several possible representations
POSIX (and probably C2x) further constrains things:
char: exactly 8 bits
short: exactly 16 bits
int: either 16 or 32 bits
long: either 32 or 64 bits
long long: 64 bits, mandatory
signed values use twos-complement representation
So I'm not sure whether it is even possible for a theoretical platform
where int is larger than uint_least32_t, for the latter to promote to
int. I _do_ know, however, that UINT8_C(n) must promote to signed int
(and that for a while, various platforms had bugging libc headers that
produced unsigned constants, which had to be worked around in gnulib),
so I am not a particular fan of using the UINTx_C() macros when not
absolutely necessary.
Looks good to me otherwise:
Reviewed-by: Laszlo Ersek <lersek(a)redhat.com>
Series and prerequisite patches pushed as 61e88a65..122322cd
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org