[PATCH nbdkit v2] common: Introduce round up, down; and divide round
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
On 9/17/18 4:05 PM, Eric Blake wrote:
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).
Nir is correct that the C standard states that ~, &, and | on signed values have implementation-defined behavior (it is only undefined once you move over to << and >>). If you are computing & or | where both values are positive, the results are well-defined. But if either operand is negative, you can get odd results. Similarly, ~ is always implementation-defined on signed (it is well-defined to flip all bits, but unless you know if the implementation defined ints as sign-magnitude, ones-complement, or twos-complement, you can end up with a different result than intended. If you don't want to assume twos-complement, then you could write it as: ~((n)-1U) to force coercion to unsigned for most inputs, at which point ~ is well-defined. Except that (int64_t - 1U) doesn't coerce to unsigned. And you don't want to blindly write 1ULL to force 64-bit math everywhere, when the macro is otherwise fine on 32-bit inputs. You can use the fact that '1 ? a : b' produces the expression 'a' coerced to the wider type of either 'a' or 'b'. And there's gcc's typeof operator, since you're already relying on extensions. (Note, I'm trying to give you enough hints to be able to write a working patch, without submitting it myself, because I'm worried that I'm perhaps tainted license-wise on this particular issue since I patched a similar bug in qemu where the license does not permit wholesale copying over to nbdkit) -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
participants (2)
-
Eric Blake -
Richard W.M. Jones