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