Libguestfs - Sep 2018 - Re: [PATCH nbdkit v2] common: Introduce round up, down; and divide round up functions.

Since we're using ({ .. }) gcc/clang extension, let's rewrite the
rounding.h change too.

Rich.

These are used at various places in the code already.  This
refactoring simply moves them to a common header file and should have
no other effect.
---
 common/include/rounding.h   | 59 +++++++++++++++++++++++++++++++++++++
 filters/cache/Makefile.am   |  3 +-
 filters/cache/cache.c       |  2 +-
 filters/cow/Makefile.am     |  3 +-
 filters/cow/cow.c           |  2 +-
 filters/truncate/truncate.c |  5 ++--
 6 files changed, 68 insertions(+), 6 deletions(-)

diff --git a/common/include/rounding.h b/common/include/rounding.h
new file mode 100644
index 0000000..b176fc3
--- /dev/null
+++ b/common/include/rounding.h
@@ -0,0 +1,59 @@
+/* nbdkit
+ * Copyright (C) 2018 Red Hat Inc.
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions are
+ * met:
+ *
+ * * Redistributions of source code must retain the above copyright
+ * notice, this list of conditions and the following disclaimer.
+ *
+ * * Redistributions in binary form must reproduce the above copyright
+ * notice, this list of conditions and the following disclaimer in the
+ * documentation and/or other materials provided with the distribution.
+ *
+ * * Neither the name of Red Hat nor the names of its contributors may be
+ * used to endorse or promote products derived from this software without
+ * specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS
IS'' AND
+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
+ * THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
+ * PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR
+ * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF
+ * USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
+ * ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
+ * OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
+ * OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+ * SUCH DAMAGE.
+ */
+
+#ifndef NBDKIT_ROUNDING_H
+#define NBDKIT_ROUNDING_H
+
+#include <stdint.h>
+#include <assert.h>
+
+#include "ispowerof2.h"
+
+/* Round up i to next multiple of n (n must be a power of 2).
+ */
+#define ROUND_UP(i, n) ({                          \
+      assert (is_power_of_2 (n));                  \
+      ((i) + (n) - 1) & ~((n) - 1);                \
+})
+
+/* Round down i to next multiple of n (n must be a power of 2).
+ */
+#define ROUND_DOWN(i, n) ({                         \
+      assert (is_power_of_2 (n));                   \
+      (i) & ~((n) - 1);                             \
+})
+
+/* Return n / d, rounding the result up to the next integer. */
+#define DIV_ROUND_UP(n, d) (((n) + (d) - 1) / (d))
+
+#endif /* NBDKIT_ROUNDING_H */
diff --git a/filters/cache/Makefile.am b/filters/cache/Makefile.am
index 867812e..827ff17 100644
--- a/filters/cache/Makefile.am
+++ b/filters/cache/Makefile.am
@@ -41,7 +41,8 @@ nbdkit_cache_filter_la_SOURCES = \
 	$(top_srcdir)/include/nbdkit-filter.h
 
 nbdkit_cache_filter_la_CPPFLAGS = \
-	-I$(top_srcdir)/include
+	-I$(top_srcdir)/include \
+	-I$(top_srcdir)/common/include
 nbdkit_cache_filter_la_CFLAGS = \
 	$(WARNINGS_CFLAGS)
 nbdkit_cache_filter_la_LDFLAGS = \
diff --git a/filters/cache/cache.c b/filters/cache/cache.c
index 90a0444..6ae6a4a 100644
--- a/filters/cache/cache.c
+++ b/filters/cache/cache.c
@@ -52,7 +52,7 @@
 
 #include <nbdkit-filter.h>
 
-#define DIV_ROUND_UP(n, d) (((n) + (d) - 1) / (d))
+#include "rounding.h"
 
 /* XXX See design comment in filters/cow/cow.c. */
 #define THREAD_MODEL NBDKIT_THREAD_MODEL_SERIALIZE_ALL_REQUESTS
diff --git a/filters/cow/Makefile.am b/filters/cow/Makefile.am
index 778df1e..c81b41c 100644
--- a/filters/cow/Makefile.am
+++ b/filters/cow/Makefile.am
@@ -41,7 +41,8 @@ nbdkit_cow_filter_la_SOURCES = \
 	$(top_srcdir)/include/nbdkit-filter.h
 
 nbdkit_cow_filter_la_CPPFLAGS = \
-	-I$(top_srcdir)/include
+	-I$(top_srcdir)/include \
+	-I$(top_srcdir)/common/include
 nbdkit_cow_filter_la_CFLAGS = \
 	$(WARNINGS_CFLAGS)
 nbdkit_cow_filter_la_LDFLAGS = \
diff --git a/filters/cow/cow.c b/filters/cow/cow.c
index 5df1db2..001d5bf 100644
--- a/filters/cow/cow.c
+++ b/filters/cow/cow.c
@@ -89,7 +89,7 @@
 
 #include <nbdkit-filter.h>
 
-#define DIV_ROUND_UP(n, d) (((n) + (d) - 1) / (d))
+#include "rounding.h"
 
 /* XXX See design comment above. */
 #define THREAD_MODEL NBDKIT_THREAD_MODEL_SERIALIZE_ALL_REQUESTS
diff --git a/filters/truncate/truncate.c b/filters/truncate/truncate.c
index 185f6cc..b95432a 100644
--- a/filters/truncate/truncate.c
+++ b/filters/truncate/truncate.c
@@ -46,6 +46,7 @@
 
 #include "ispowerof2.h"
 #include "iszero.h"
+#include "rounding.h"
 
 #define THREAD_MODEL NBDKIT_THREAD_MODEL_PARALLEL
 
@@ -157,9 +158,9 @@ truncate_get_size (struct nbdkit_next_ops *next_ops, void
*nxdata,
   if (truncate_size >= 0)
     size = truncate_size;
   if (round_up > 0)
-    size = (size + round_up - 1) & ~(round_up - 1);
+    size = ROUND_UP (size, round_up);
   if (round_down > 0)
-    size &= ~(round_down - 1);
+    size = ROUND_DOWN (size, round_down);
   ret = size;
 
   pthread_mutex_unlock (&lock);
-- 
2.19.0.rc0

On 9/17/18 3:38 PM, Richard W.M. Jones wrote:
> These are used at various places in the code already.  This
> refactoring simply moves them to a common header file and should have
> no other effect.
> ---
>   common/include/rounding.h   | 59 +++++++++++++++++++++++++++++++++++++
>   filters/cache/Makefile.am   |  3 +-
>   filters/cache/cache.c       |  2 +-
>   filters/cow/Makefile.am     |  3 +-
>   filters/cow/cow.c           |  2 +-
>   filters/truncate/truncate.c |  5 ++--
>   6 files changed, 68 insertions(+), 6 deletions(-)
> 
> +/* Round up i to next multiple of n (n must be a power of 2).
> + */
> +#define ROUND_UP(i, n) ({                          \
> +      assert (is_power_of_2 (n));                  \
> +      ((i) + (n) - 1) & ~((n) - 1);                \
> +})
Potential bug: if n is 32-bit unsigned, but i is 64-bit, the bitmask on 
the right half of & will be inappropriately truncated (no sign extension 
is performed).
> +
> +/* Round down i to next multiple of n (n must be a power of 2).
> + */
> +#define ROUND_DOWN(i, n) ({                         \
> +      assert (is_power_of_2 (n));                   \
> +      (i) & ~((n) - 1);                             \
> +})
And again.

Also, ~((n)-1) is identical to -(n), if you want less typing (well, as 
long as you assume twos-complements signed numbers, but that's pretty 
safe to assume these days).
> +
> +/* Return n / d, rounding the result up to the next integer. */
> +#define DIV_ROUND_UP(n, d) (((n) + (d) - 1) / (d))
This one should be okay, though.
> +++ b/filters/truncate/truncate.c
> @@ -46,6 +46,7 @@
>   
>   #include "ispowerof2.h"
>   #include "iszero.h"
> +#include "rounding.h"
>   
>   #define THREAD_MODEL NBDKIT_THREAD_MODEL_PARALLEL
>   
> @@ -157,9 +158,9 @@ truncate_get_size (struct nbdkit_next_ops *next_ops,
void *nxdata,
>     if (truncate_size >= 0)
>       size = truncate_size;
>     if (round_up > 0)
> -    size = (size + round_up - 1) & ~(round_up - 1);
> +    size = ROUND_UP (size, round_up);
>     if (round_down > 0)
> -    size &= ~(round_down - 1);
> +    size = ROUND_DOWN (size, round_down);
Ouch - the bug was pre-existing.  size is uint64_t, while 
round_{up,down} is unsigned (and hence 32-bit), which means this failed 
on sizes larger than 4G.

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