On Thu, Mar 23, 2023 at 01:10:00PM +0100, Laszlo Ersek wrote: > F_SETFD takes an "int", so it stands to reason that FD_CLOEXEC expands to > an "int". In turn, it's bad hygiene to manipulate the sign bit of (signed) > integers with bit operations: > > ~FD_CLOEXEC
Hmm - I've never really programmed on a system with ones' complement encoding, but you a right that in C99, the ~ operator on a signed value (even where we know the original value is positive) is bad hygeine because it creates a result whose value is dependent on whether the encoding is the usual two's complement or not; furthermore, using a signed argument to & is risky. Note, however, that POSIX issue 8 (adding a restriction beyond on C17; https://www.austingroupbugs.net/view.php?id=1108#c4094), as well as C23 (http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2412.pdf) will require two's complement encoding, at which point what used to be bad hygiene is now entirely predictable. > > Convert FD_CLOEXEC to "unsigned int" for the bitwise complement operator: > > ~(unsigned)FD_CLOEXEC > > The bitwise complement then results in an "unsigned int". "Flags" (of type > "int", and having, per POSIX, a non-negative value returned by > fcntl(F_GETFD)) will be automatically converted to "unsigned int" by the > usual arithmetic conversions for the bitwise AND operator: > > flags & ~(unsigned)FD_CLOEXEC > > We could pass the result of *that* to fcntl(), due to (a) the value being > in range for "signed int" ("flags" is a non-negative "int", and we're only > clearing a value bit), and (b) non-negative "int" values being represented > identically by "unsigned int" (C99 6.2.5 p9). But, for clarity, let's cast > the result to "int" explicitly: > > (int)(flags & ~(unsigned)FD_CLOEXEC) Rather verbose. If you have evidence of a current sanitizing compiler that warns about the short construct (at compile- or run-time), that would be a definitive reason to do this. But given that future standards will require the short form to work identically to the long form, and I'm unaware of a compiler that actually warns on the short form, I'm 50-50 on whether to take this one. It's not technically wrong, just not compelling. But since it is only one line, it's easy to maintain, so if you still want to include it, I don't mind if you add: Reviewed-by: Eric Blake <ebl...@redhat.com> -- 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
