Re: [Libguestfs] [libnbd PATCH v3 03/19] socket activation: avoid manipulating the sign bit
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 <eblake(a)redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org