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@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