[Libguestfs] [PATCH nbdkit v2] common: isaligned: Use a macro instead of relying on implicit truncation.

[Eric Blake]

On 9/17/18 3:27 PM, Richard W.M. Jones wrote:
> ---
>   common/include/isaligned.h | 11 +++++------
>   plugins/file/file.c        |  4 ++--
>   plugins/vddk/vddk.c        |  8 ++++----
>   3 files changed, 11 insertions(+), 12 deletions(-)
> 
> diff --git a/common/include/isaligned.h b/common/include/isaligned.h
> index e693820..81ce8a7 100644
> --- a/common/include/isaligned.h
> +++ b/common/include/isaligned.h
> @@ -36,16 +36,15 @@
>   
>   #include <assert.h>
>   #include <stdbool.h>
> +#include <stdint.h>
>   
>   #include "ispowerof2.h"
>   
>   /* Return true if size is a multiple of align. align must be power of 2.
>    */
> -static inline bool
> -is_aligned (unsigned int size, unsigned int align)
> -{
> -  assert (is_power_of_2 (align));
> -  return !(size & (align - 1));

Not just implicit truncation, but implicit promotion to unsigned.

> -}
> +#define IS_ALIGNED(size, align) ({              \
> +      assert (is_power_of_2 ((align)));         \
> +      !((size) & ((align) - 1));                \

This, on the other hand, makes me worry if it can do weird things if 
size or align is signed.  (I guess I've been burned by qemu commit 
2098b073 in the past).  Thankfully, even if you mix 32- and 64-bit types 
as your two inputs, I can't see how this would cause weird sign 
extension effects unless align was 0 (which is already a violation of 
the is_power_of_2() assertion).

This evaluates 'align' twice, hence the rename to all caps; as long as 
you are using extensions, you could also use typeof and assign to a 
temporary to guarantee only single evaluation of 'align'.  But none of 
the callers were impacted, so going with the simpler version is fine (if 
we later have a caller where side effects matter, we can beef up the 
macro at that point).

Looks fine to me.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org