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

---
 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));
-}
+#define IS_ALIGNED(size, align) ({              \
+      assert (is_power_of_2 ((align)));         \
+      !((size) & ((align) - 1));                \
+})
 
 #endif /* NBDKIT_ISALIGNED_H */
diff --git a/plugins/file/file.c b/plugins/file/file.c
index 9d03f18..1391f62 100644
--- a/plugins/file/file.c
+++ b/plugins/file/file.c
@@ -397,13 +397,13 @@ file_zero (void *handle, uint32_t count, uint64_t offset,
int may_trim)
 
 #ifdef BLKZEROOUT
   /* For aligned range and block device, we can use BLKZEROOUT. */
-  if (h->can_zeroout && is_aligned (offset | count,
h->sector_size)) {
+  if (h->can_zeroout && IS_ALIGNED (offset | count,
h->sector_size)) {
     uint64_t range[2] = {offset, count};
 
     r = ioctl (h->fd, BLKZEROOUT, &range);
     if (r == 0) {
       if (file_debug_zero)
-        nbdkit_debug ("h->can_zeroout && is_aligned: "
+        nbdkit_debug ("h->can_zeroout && IS_ALIGNED: "
                       "zero succeeded using BLKZEROOUT");
       return 0;
     }
diff --git a/plugins/vddk/vddk.c b/plugins/vddk/vddk.c
index f3b4539..9bfd4d2 100644
--- a/plugins/vddk/vddk.c
+++ b/plugins/vddk/vddk.c
@@ -511,11 +511,11 @@ vddk_pread (void *handle, void *buf, uint32_t count,
uint64_t offset)
   VixError err;
 
   /* Align to sectors. */
-  if (!is_aligned (offset, VIXDISKLIB_SECTOR_SIZE)) {
+  if (!IS_ALIGNED (offset, VIXDISKLIB_SECTOR_SIZE)) {
     nbdkit_error ("read is not aligned to sectors");
     return -1;
   }
-  if (!is_aligned (count, VIXDISKLIB_SECTOR_SIZE)) {
+  if (!IS_ALIGNED (count, VIXDISKLIB_SECTOR_SIZE)) {
     nbdkit_error ("read is not aligned to sectors");
     return -1;
   }
@@ -544,11 +544,11 @@ vddk_pwrite (void *handle, const void *buf, uint32_t
count, uint64_t offset)
   VixError err;
 
   /* Align to sectors. */
-  if (!is_aligned (offset, VIXDISKLIB_SECTOR_SIZE)) {
+  if (!IS_ALIGNED (offset, VIXDISKLIB_SECTOR_SIZE)) {
     nbdkit_error ("read is not aligned to sectors");
     return -1;
   }
-  if (!is_aligned (count, VIXDISKLIB_SECTOR_SIZE)) {
+  if (!IS_ALIGNED (count, VIXDISKLIB_SECTOR_SIZE)) {
     nbdkit_error ("read is not aligned to sectors");
     return -1;
   }
-- 
2.19.0.rc0

On Mon, Sep 17, 2018 at 11:27 PM Richard W.M. Jones <rjones@redhat.com>
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>
>
Not need with this change...

>
>  #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));
> -}
> +#define IS_ALIGNED(size, align) ({              \
> +      assert (is_power_of_2 ((align)));         \
> +      !((size) & ((align) - 1));                \
> +})
>
But this version will happily accept singed int, and I think this code
behavior with signed int is undefined.

See
https://wiki.sei.cmu.edu/confluence/display/c/INT13-C.+Use+bitwise+operators+only+on+unsigned+operands

Why use a macro when we can use a function that is likely to be inlined
anyway?
This is not used in tight loops, so there is no reason to use a macro.

If you want to use a macro, see the kernel align macros:

  57 /* @a is a power of 2 value */
  58 #define ALIGN(x, a)     __ALIGN_KERNEL((x), (a))
  59 #define ALIGN_DOWN(x, a)    __ALIGN_KERNEL((x) - ((a) - 1), (a))
  60 #define __ALIGN_MASK(x, mask)   __ALIGN_KERNEL_MASK((x), (mask))
  61 #define PTR_ALIGN(p, a)     ((typeof(p))ALIGN((unsigned long)(p), (a)))
  62 #define IS_ALIGNED(x, a)        (((x) & ((typeof(x))(a) - 1)) == 0)

>
>  #endif /* NBDKIT_ISALIGNED_H */
> diff --git a/plugins/file/file.c b/plugins/file/file.c
> index 9d03f18..1391f62 100644
> --- a/plugins/file/file.c
> +++ b/plugins/file/file.c
> @@ -397,13 +397,13 @@ file_zero (void *handle, uint32_t count, uint64_t
> offset, int may_trim)
>
>  #ifdef BLKZEROOUT
>    /* For aligned range and block device, we can use BLKZEROOUT. */
> -  if (h->can_zeroout && is_aligned (offset | count,
h->sector_size)) {
> +  if (h->can_zeroout && IS_ALIGNED (offset | count,
h->sector_size)) {
>      uint64_t range[2] = {offset, count};
>
>      r = ioctl (h->fd, BLKZEROOUT, &range);
>      if (r == 0) {
>        if (file_debug_zero)
> -        nbdkit_debug ("h->can_zeroout && is_aligned:
"
> +        nbdkit_debug ("h->can_zeroout && IS_ALIGNED:
"
>                        "zero succeeded using BLKZEROOUT");
>        return 0;
>      }
> diff --git a/plugins/vddk/vddk.c b/plugins/vddk/vddk.c
> index f3b4539..9bfd4d2 100644
> --- a/plugins/vddk/vddk.c
> +++ b/plugins/vddk/vddk.c
> @@ -511,11 +511,11 @@ vddk_pread (void *handle, void *buf, uint32_t count,
> uint64_t offset)
>    VixError err;
>
>    /* Align to sectors. */
> -  if (!is_aligned (offset, VIXDISKLIB_SECTOR_SIZE)) {
> +  if (!IS_ALIGNED (offset, VIXDISKLIB_SECTOR_SIZE)) {
>      nbdkit_error ("read is not aligned to sectors");
>      return -1;
>    }
> -  if (!is_aligned (count, VIXDISKLIB_SECTOR_SIZE)) {
> +  if (!IS_ALIGNED (count, VIXDISKLIB_SECTOR_SIZE)) {
>      nbdkit_error ("read is not aligned to sectors");
>      return -1;
>    }
> @@ -544,11 +544,11 @@ vddk_pwrite (void *handle, const void *buf, uint32_t
> count, uint64_t offset)
>    VixError err;
>
>    /* Align to sectors. */
> -  if (!is_aligned (offset, VIXDISKLIB_SECTOR_SIZE)) {
> +  if (!IS_ALIGNED (offset, VIXDISKLIB_SECTOR_SIZE)) {
>      nbdkit_error ("read is not aligned to sectors");
>      return -1;
>    }
> -  if (!is_aligned (count, VIXDISKLIB_SECTOR_SIZE)) {
> +  if (!IS_ALIGNED (count, VIXDISKLIB_SECTOR_SIZE)) {
>      nbdkit_error ("read is not aligned to sectors");
>      return -1;
>    }
> --
> 2.19.0.rc0
>
>

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

On Mon, Sep 17, 2018 at 11:39:30PM +0300, Nir Soffer
wrote:
> But this version will happily accept singed int, and I think this code
> behavior with signed int is undefined.
[...]

Yes this is certainly a problem which I didn't foresee.  What do you
think of Eric's comment on the patch?

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/

On 9/17/18 3:39 PM, Nir Soffer wrote:
>> +#define IS_ALIGNED(size, align) ({              \
>> +      assert (is_power_of_2 ((align)));         \
>> +      !((size) & ((align) - 1));                \
>> +})
>>
> 
> But this version will happily accept singed int, and I think this code
> behavior with signed int is undefined.
Well, sort of. Bit shifts are very likely to hit undefined behavior on 
ints. But & and | are not shifts.
> 
> See
>
https://wiki.sei.cmu.edu/confluence/display/c/INT13-C.+Use+bitwise+operators+only+on+unsigned+operands
> 
> Why use a macro when we can use a function that is likely to be inlined
> anyway?
> This is not used in tight loops, so there is no reason to use a macro.
> 
> If you want to use a macro, see the kernel align macros:
Alas, the kernel has a more restrictive license (GPLv2 in general); 
which means we can't copy it into nbdkit.  (True, the definitions will 
look similar, but if we claim we copied from somewhere, that source had 
better be permissively licensed).

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