[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... 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 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...
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
On Tue, Sep 18, 2018 at 12:13 AM Eric Blake <eblake@redhat.com> wrote:
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...
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).
The FreeBSD version: #define IS_ALIGNED <http://fxr.watson.org/fxr/ident?i=IS_ALIGNED>(n <http://fxr.watson.org/fxr/ident?i=n>,align) (!((uint32_t)(n <http://fxr.watson.org/fxr/ident?i=n>) & (align - 1))) http://fxr.watson.org/fxr/source/contrib/ncsw/inc/ncsw_ext.h#L182 Nir
On 9/17/18 4:41 PM, Nir Soffer wrote:
The FreeBSD version:
#define IS_ALIGNED <http://fxr.watson.org/fxr/ident?i=IS_ALIGNED>(n <http://fxr.watson.org/fxr/ident?i=n>,align) (!((uint32_t)(n <http://fxr.watson.org/fxr/ident?i=n>) & (align - 1)))
http://fxr.watson.org/fxr/source/contrib/ncsw/inc/ncsw_ext.h#L182
Which truncates to 32 bits. But that's what we're trying to get rid of - by moving to a macro that lets us do full 64-bit alignment if needed. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
On Tue, Sep 18, 2018 at 12:50 AM Eric Blake <eblake@redhat.com> wrote:
On 9/17/18 4:41 PM, Nir Soffer wrote:
The FreeBSD version:
#define IS_ALIGNED <http://fxr.watson.org/fxr/ident?i=IS_ALIGNED>(n <http://fxr.watson.org/fxr/ident?i=n>,align) (!((uint32_t)(n <http://fxr.watson.org/fxr/ident?i=n>) & (align - 1)))
http://fxr.watson.org/fxr/source/contrib/ncsw/inc/ncsw_ext.h#L182
Which truncates to 32 bits. But that's what we're trying to get rid of - by moving to a macro that lets us do full 64-bit alignment if needed.
My point is that we don't need to use full 64-bit, so there is nothing to get rid of. Nir
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
participants (3)
-
Eric Blake -
Nir Soffer -
Richard W.M. Jones