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 <ler...@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 _______________________________________________ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
