[Libguestfs] [libnbd PATCH] api: Generate flag values as unsigned

[Richard W.M. Jones]

On Fri, Nov 11, 2022 at 11:27:51AM -0600, Eric Blake wrote:
> C says that performing bitwise math on ints can lead to corner-case
> undefined values.  When we document something as being usable as a
> flag with bitwise-OR and similar, we should ensure the values are
> unsigned, to avoid those corner-cases in C.  Even our own codebase was
> potentially falling foul of strict C rules, with code like
> tests/errors-server-invalid-offset.c:
> 
>   uint32_t strict;
>   strict = nbd_get_strict_mode (nbd) & ~LIBNBD_STRICT_BOUNDS;
> 
> In theory, this is an API change that might be detectable by someone
> trying to probe which integer promotions occur on a given constant.
> In practice, no one writes code that intentionally tries to exploit
> whether a flag is signed or unsigned, and none of our values were
> large enough to worry about sign-extension effects that occur when
> flipping the sign bit of a signed value (we tend to target machines
> with twos-complement signed values where the behavior is sane despite
> being undefined by C), so this patch is therefore not an ABI change.
> 
> Generated code changes as follows:
> 
> | --- include/libnbd.h.bak	2022-11-11 11:15:54.929686924 -0600
> | +++ include/libnbd.h	2022-11-11 11:15:56.652698919 -0600
> | @@ -69,32 +69,32 @@
> |  #define LIBNBD_SIZE_MAXIMUM                      2
> |  #define LIBNBD_SIZE_PAYLOAD                      3
> |
> | -#define LIBNBD_CMD_FLAG_FUA                      0x01
> | -#define LIBNBD_CMD_FLAG_NO_HOLE                  0x02
> | ...
> | -#define LIBNBD_ALLOW_TRANSPORT_VSOCK             0x04
> | -#define LIBNBD_ALLOW_TRANSPORT_MASK              0x07
> | +#define LIBNBD_CMD_FLAG_FUA                      0x01U
> | +#define LIBNBD_CMD_FLAG_NO_HOLE                  0x02U
> | ...
> | +#define LIBNBD_ALLOW_TRANSPORT_VSOCK             0x04U
> | +#define LIBNBD_ALLOW_TRANSPORT_MASK              0x07U
> 
> Thanks: Laszlo Ersek <lersek at redhat.com>
> ---
> 
> I'll push this along with the LIBNBD_STRICT_PAYLOAD series.
> 
>  generator/C.ml | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/generator/C.ml b/generator/C.ml
> index cfa2964c..f9171996 100644
> --- a/generator/C.ml
> +++ b/generator/C.ml
> @@ -437,11 +437,11 @@ let
>        List.iter (
>          fun (flag, i) ->
>            let flag = sprintf "LIBNBD_%s_%s" flag_prefix flag in
> -          pr "#define %-40s 0x%02x\n" flag i;
> +          pr "#define %-40s 0x%02xU\n" flag i;
>            mask := !mask lor i
>        ) flags;
>        let flag = sprintf "LIBNBD_%s_MASK" flag_prefix in
> -      pr "#define %-40s 0x%02x\n" flag !mask;
> +      pr "#define %-40s 0x%02xU\n" flag !mask;
>        pr "\n"
>    ) all_flags;
>    List.iter (
> -- 

Looks good, thanks.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org