3:38 p.m.
Since we're using ({ .. }) gcc/clang extension, let's rewrite the
rounding.h change too.
Rich.
3:38 p.m.
New subject: [PATCH nbdkit v2] common: Introduce round up, down; and divide round up functions.
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
4:05 p.m.
New subject: [PATCH nbdkit v2] common: Introduce round up, down; and divide round up functions.
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
4:27 p.m.
New subject: [PATCH nbdkit v2] common: Introduce round up, down; and divide round up functions.
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
2292
days inactive
2292
days old
3 comments
2 participants
participants (2)
-
Eric Blake -
Richard W.M. Jones