Richard W.M. Jones
2018-Sep-17 20:27 UTC
[Libguestfs] [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
Nir Soffer
2018-Sep-17 20:39 UTC
Re: [Libguestfs] [PATCH nbdkit v2] common: isaligned: Use a macro instead of relying on implicit truncation.
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 > >
Eric Blake
2018-Sep-17 20:39 UTC
Re: [Libguestfs] [PATCH nbdkit v2] common: isaligned: Use a macro instead of relying on implicit truncation.
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
Richard W.M. Jones
2018-Sep-17 20:58 UTC
Re: [Libguestfs] [PATCH nbdkit v2] common: isaligned: Use a macro instead of relying on implicit truncation.
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/
Eric Blake
2018-Sep-17 21:12 UTC
Re: [Libguestfs] [PATCH nbdkit v2] common: isaligned: Use a macro instead of relying on implicit truncation.
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
Reasonably Related Threads
- [PATCH nbdkit v3 1/3] common: isaligned: Use a macro instead of relying on implicit truncation.
- Re: [PATCH nbdkit v2] common: isaligned: Use a macro instead of relying on implicit truncation.
- [PATCH nbdkit v3 0/3] Add partitioning plugin.
- [PATCH v3 0/4] file: Zero for block devices and older file systems
- [PATCH v3 0/4] file: Zero for block devices and older file systems