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

[Nir Soffer]

On Mon, Sep 17, 2018 at 11:27 PM Richard W.M. Jones <rjones at 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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libguestfs/attachments/20180917/9dd37af4/attachment.htm>