10:22 a.m.
This is the first part of a pair of patch series which aim to let us
use nbdkit_parse_size (or rather, an equivalent common function) in
nbdcopy, so we can write:
nbdcopy --request-size=32M ...
We can't do that now which was annoying me earlier in the week.
This commit creates a new function called human_size_parse which is
basically nbdkit_parse_size, and turns nbdkit_parse_size into a
wrapper around it.
Rich.
10:22 a.m.
This is broadly simple code motion, intended so that we can reuse the
same code in libnbd.
---
common/include/Makefile.am | 6 ++
common/include/human-size.h | 137 +++++++++++++++++++++++++++++++
common/include/test-human-size.c | 133 ++++++++++++++++++++++++++++++
server/public.c | 78 ++----------------
.gitignore | 1 +
5 files changed, 283 insertions(+), 72 deletions(-)
diff --git a/common/include/Makefile.am b/common/include/Makefile.am
index 3162e92c2..8e4de04f3 100644
--- a/common/include/Makefile.am
+++ b/common/include/Makefile.am
@@ -42,6 +42,7 @@ EXTRA_DIST = \
checked-overflow.h \
compiler-macros.h \
hexdigit.h \
+ human-size.h \
isaligned.h \
ispowerof2.h \
iszero.h \
@@ -63,6 +64,7 @@ TESTS = \
test-ascii-string \
test-byte-swapping \
test-checked-overflow \
+ test-human-size \
test-isaligned \
test-ispowerof2 \
test-iszero \
@@ -93,6 +95,10 @@ test_checked_overflow_SOURCES = test-checked-overflow.c
checked-overflow.h
test_checked_overflow_CPPFLAGS = -I$(srcdir)
test_checked_overflow_CFLAGS = $(WARNINGS_CFLAGS)
+test_human_size_SOURCES = test-human-size.c human-size.h
+test_human_size_CPPFLAGS = -I$(srcdir)
+test_human_size_CFLAGS = $(WARNINGS_CFLAGS)
+
test_isaligned_SOURCES = test-isaligned.c isaligned.h
test_isaligned_CPPFLAGS = -I$(srcdir)
test_isaligned_CFLAGS = $(WARNINGS_CFLAGS)
diff --git a/common/include/human-size.h b/common/include/human-size.h
new file mode 100644
index 000000000..788dbd7ba
--- /dev/null
+++ b/common/include/human-size.h
@@ -0,0 +1,137 @@
+/* nbdkit
+ * Copyright Red Hat
+ *
+ * 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_HUMAN_SIZE_H
+#define NBDKIT_HUMAN_SIZE_H
+
+#include <stdint.h>
+#include <inttypes.h>
+#include <errno.h>
+
+/* Attempt to parse a string with a possible scaling suffix, such as
+ * "2M". Disk sizes cannot usefully exceed off_t (which is signed)
+ * and cannot be negative.
+ *
+ * On error, returns -1 and sets *error and *pstr. You can form a
+ * final error message by appending "<error>: <pstr>".
+ */
+static inline int64_t
+human_size_parse (const char *str,
+ const char **error, const char **pstr)
+{
+ int64_t size;
+ char *end;
+ uint64_t scale = 1;
+
+ /* XXX Should we also parse things like '1.5M'? */
+ /* XXX Should we allow hex? If so, hex cannot use scaling suffixes,
+ * because some of them are valid hex digits.
+ */
+ errno = 0;
+ size = strtoimax (str, &end, 10);
+ if (str == end) {
+ *error = "could not parse size string";
+ *pstr = str;
+ return -1;
+ }
+ if (size < 0) {
+ *error = "size cannot be negative";
+ *pstr = str;
+ return -1;
+ }
+ if (errno) {
+ *error = "size exceeds maximum value";
+ *pstr = str;
+ return -1;
+ }
+
+ switch (*end) {
+ /* No suffix */
+ case '\0':
+ end--; /* Safe, since we already filtered out empty string */
+ break;
+
+ /* Powers of 1024 */
+ case 'e': case 'E':
+ scale *= 1024;
+ /* fallthru */
+ case 'p': case 'P':
+ scale *= 1024;
+ /* fallthru */
+ case 't': case 'T':
+ scale *= 1024;
+ /* fallthru */
+ case 'g': case 'G':
+ scale *= 1024;
+ /* fallthru */
+ case 'm': case 'M':
+ scale *= 1024;
+ /* fallthru */
+ case 'k': case 'K':
+ scale *= 1024;
+ /* fallthru */
+ case 'b': case 'B':
+ break;
+
+ /* "sectors", ie. units of 512 bytes, even if that's not the real
+ * sector size
+ */
+ case 's': case 'S':
+ scale = 512;
+ break;
+
+ default:
+ *error = "could not parse size: unknown suffix";
+ *pstr = end;
+ return -1;
+ }
+
+ /* XXX Maybe we should support 'MiB' as a synonym for 'M'; and
'MB'
+ * for powers of 1000, for similarity to GNU tools. But for now,
+ * anything beyond 'M' is dropped.
+ */
+ if (end[1]) {
+ *error = "could not parse size: unknown suffix";
+ *pstr = end;
+ return -1;
+ }
+
+ if (INT64_MAX / scale < size) {
+ *error = "could not parse size: size * scale overflows";
+ *pstr = str;
+ return -1;
+ }
+
+ return size * scale;
+}
+
+#endif /* NBDKIT_HUMAN_SIZE_H */
diff --git a/common/include/test-human-size.c b/common/include/test-human-size.c
new file mode 100644
index 000000000..e8cbe7f41
--- /dev/null
+++ b/common/include/test-human-size.c
@@ -0,0 +1,133 @@
+/* nbdkit
+ * Copyright Red Hat
+ *
+ * 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.
+ */
+
+#include <config.h>
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <stdbool.h>
+#include <stdint.h>
+
+#include "array-size.h"
+#include "human-size.h"
+
+int
+main (void)
+{
+ size_t i;
+ bool pass = true;
+ struct pair {
+ const char *str;
+ int64_t res;
+ } pairs[] = {
+ /* Bogus strings */
+ { "", -1 },
+ { "0x0", -1 },
+ { "garbage", -1 },
+ { "0garbage", -1 },
+ { "8E", -1 },
+ { "8192P", -1 },
+
+ /* Strings leading to overflow */
+ { "9223372036854775808", -1 }, /* INT_MAX + 1 */
+ { "18446744073709551614", -1 }, /* UINT64_MAX - 1 */
+ { "18446744073709551615", -1 }, /* UINT64_MAX */
+ { "18446744073709551616", -1 }, /* UINT64_MAX + 1 */
+ { "999999999999999999999999", -1 },
+
+ /* Strings representing negative values */
+ { "-1", -1 },
+ { "-2", -1 },
+ { "-9223372036854775809", -1 }, /* INT64_MIN - 1 */
+ { "-9223372036854775808", -1 }, /* INT64_MIN */
+ { "-9223372036854775807", -1 }, /* INT64_MIN + 1 */
+ { "-18446744073709551616", -1 }, /* -UINT64_MAX - 1 */
+ { "-18446744073709551615", -1 }, /* -UINT64_MAX */
+ { "-18446744073709551614", -1 }, /* -UINT64_MAX + 1 */
+
+ /* Strings we may want to support in the future */
+ { "M", -1 },
+ { "1MB", -1 },
+ { "1MiB", -1 },
+ { "1.5M", -1 },
+
+ /* Valid strings */
+ { "-0", 0 },
+ { "0", 0 },
+ { "+0", 0 },
+ { " 08", 8 },
+ { "1", 1 },
+ { "+1", 1 },
+ { "1234567890", 1234567890 },
+ { "+1234567890", 1234567890 },
+ { "9223372036854775807", INT64_MAX },
+ { "1s", 512 },
+ { "2S", 1024 },
+ { "1b", 1 },
+ { "1B", 1 },
+ { "1k", 1024 },
+ { "1K", 1024 },
+ { "1m", 1024 * 1024 },
+ { "1M", 1024 * 1024 },
+ { "+1M", 1024 * 1024 },
+ { "1g", 1024 * 1024 * 1024 },
+ { "1G", 1024 * 1024 * 1024 },
+ { "1t", 1024LL * 1024 * 1024 * 1024 },
+ { "1T", 1024LL * 1024 * 1024 * 1024 },
+ { "1p", 1024LL * 1024 * 1024 * 1024 * 1024 },
+ { "1P", 1024LL * 1024 * 1024 * 1024 * 1024 },
+ { "8191p", 1024LL * 1024 * 1024 * 1024 * 1024 * 8191 },
+ { "1e", 1024LL * 1024 * 1024 * 1024 * 1024 * 1024 },
+ { "1E", 1024LL * 1024 * 1024 * 1024 * 1024 * 1024 },
+ };
+
+ for (i = 0; i < ARRAY_SIZE (pairs); i++) {
+ const char *error = NULL, *pstr = NULL;
+ int64_t r;
+
+ r = human_size_parse (pairs[i].str, &error, &pstr);
+ if (r != pairs[i].res) {
+ fprintf (stderr,
+ "Wrong parse for %s, got %" PRId64 ", expected %"
PRId64 "\n",
+ pairs[i].str, r, pairs[i].res);
+ pass = false;
+ }
+ if (r == -1) {
+ if (error == NULL || pstr == NULL) {
+ fprintf (stderr, "Wrong error message handling for %s\n",
pairs[i].str);
+ pass = false;
+ }
+ }
+ }
+
+ exit (pass ? EXIT_SUCCESS : EXIT_FAILURE);
+}
diff --git a/server/public.c b/server/public.c
index 705ac3a47..a1ba603d4 100644
--- a/server/public.c
+++ b/server/public.c
@@ -76,6 +76,7 @@
#include "ascii-string.h"
#include "get_current_dir_name.h"
#include "getline.h"
+#include "human-size.h"
#include "poll.h"
#include "realpath.h"
#include "strndup.h"
@@ -343,83 +344,16 @@ nbdkit_parse_uint64_t (const char *what, const char *str, uint64_t
*rp)
NBDKIT_DLL_PUBLIC int64_t
nbdkit_parse_size (const char *str)
{
+ const char *error, *pstr;
int64_t size;
- char *end;
- uint64_t scale = 1;
- /* Disk sizes cannot usefully exceed off_t (which is signed) and
- * cannot be negative. */
- /* XXX Should we also parse things like '1.5M'? */
- /* XXX Should we allow hex? If so, hex cannot use scaling suffixes,
- * because some of them are valid hex digits */
- errno = 0;
- size = strtoimax (str, &end, 10);
- if (str == end) {
- nbdkit_error ("could not parse size string (%s)", str);
- return -1;
- }
- if (size < 0) {
- nbdkit_error ("size cannot be negative (%s)", str);
- return -1;
- }
- if (errno) {
- nbdkit_error ("size (%s) exceeds maximum value", str);
- return -1;
- }
-
- switch (*end) {
- /* No suffix */
- case '\0':
- end--; /* Safe, since we already filtered out empty string */
- break;
-
- /* Powers of 1024 */
- case 'e': case 'E':
- scale *= 1024;
- /* fallthru */
- case 'p': case 'P':
- scale *= 1024;
- /* fallthru */
- case 't': case 'T':
- scale *= 1024;
- /* fallthru */
- case 'g': case 'G':
- scale *= 1024;
- /* fallthru */
- case 'm': case 'M':
- scale *= 1024;
- /* fallthru */
- case 'k': case 'K':
- scale *= 1024;
- /* fallthru */
- case 'b': case 'B':
- break;
-
- /* "sectors", ie. units of 512 bytes, even if that's not the real
- * sector size */
- case 's': case 'S':
- scale = 512;
- break;
-
- default:
- nbdkit_error ("could not parse size: unknown suffix '%s'", end);
- return -1;
- }
-
- /* XXX Maybe we should support 'MiB' as a synonym for 'M'; and
'MB'
- * for powers of 1000, for similarity to GNU tools. But for now,
- * anything beyond 'M' is dropped. */
- if (end[1]) {
- nbdkit_error ("could not parse size: unknown suffix '%s'", end);
- return -1;
- }
-
- if (INT64_MAX / scale < size) {
- nbdkit_error ("overflow computing size (%s)", str);
+ size = human_size_parse (str, &error, &pstr);
+ if (size == -1) {
+ nbdkit_error ("%s: %s", error, pstr);
return -1;
}
- return size * scale;
+ return size;
}
NBDKIT_DLL_PUBLIC int
diff --git a/.gitignore b/.gitignore
index 49af3998f..04fdcd723 100644
--- a/.gitignore
+++ b/.gitignore
@@ -40,6 +40,7 @@ plugins/*/*.3
/common/include/test-ascii-string
/common/include/test-byte-swapping
/common/include/test-checked-overflow
+/common/include/test-human-size
/common/include/test-isaligned
/common/include/test-ispowerof2
/common/include/test-iszero
--
2.41.0
2:52 a.m.
On 9/3/23 17:22, Richard W.M. Jones wrote:
This is broadly simple code motion, intended so that we can reuse
the
same code in libnbd.
---
common/include/Makefile.am | 6 ++
common/include/human-size.h | 137 +++++++++++++++++++++++++++++++
common/include/test-human-size.c | 133 ++++++++++++++++++++++++++++++
server/public.c | 78 ++----------------
.gitignore | 1 +
5 files changed, 283 insertions(+), 72 deletions(-)
diff --git a/common/include/Makefile.am b/common/include/Makefile.am
index 3162e92c2..8e4de04f3 100644
--- a/common/include/Makefile.am
+++ b/common/include/Makefile.am
@@ -42,6 +42,7 @@ EXTRA_DIST = \
checked-overflow.h \
compiler-macros.h \
hexdigit.h \
+ human-size.h \
isaligned.h \
ispowerof2.h \
iszero.h \
@@ -63,6 +64,7 @@ TESTS = \
test-ascii-string \
test-byte-swapping \
test-checked-overflow \
+ test-human-size \
test-isaligned \
test-ispowerof2 \
test-iszero \
@@ -93,6 +95,10 @@ test_checked_overflow_SOURCES = test-checked-overflow.c
checked-overflow.h
test_checked_overflow_CPPFLAGS = -I$(srcdir)
test_checked_overflow_CFLAGS = $(WARNINGS_CFLAGS)
+test_human_size_SOURCES = test-human-size.c human-size.h
+test_human_size_CPPFLAGS = -I$(srcdir)
+test_human_size_CFLAGS = $(WARNINGS_CFLAGS)
+
test_isaligned_SOURCES = test-isaligned.c isaligned.h
test_isaligned_CPPFLAGS = -I$(srcdir)
test_isaligned_CFLAGS = $(WARNINGS_CFLAGS)
diff --git a/common/include/human-size.h b/common/include/human-size.h
new file mode 100644
index 000000000..788dbd7ba
--- /dev/null
+++ b/common/include/human-size.h
@@ -0,0 +1,137 @@
+/* nbdkit
+ * Copyright Red Hat
+ *
+ * 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_HUMAN_SIZE_H
+#define NBDKIT_HUMAN_SIZE_H
+
+#include <stdint.h>
+#include <inttypes.h>
+#include <errno.h>
+
+/* Attempt to parse a string with a possible scaling suffix, such as
+ * "2M". Disk sizes cannot usefully exceed off_t (which is signed)
+ * and cannot be negative.
+ *
+ * On error, returns -1 and sets *error and *pstr. You can form a
+ * final error message by appending "<error>: <pstr>".
+ */
+static inline int64_t
+human_size_parse (const char *str,
+ const char **error, const char **pstr)
+{
+ int64_t size;
+ char *end;
+ uint64_t scale = 1;
+
+ /* XXX Should we also parse things like '1.5M'? */
+ /* XXX Should we allow hex? If so, hex cannot use scaling suffixes,
+ * because some of them are valid hex digits.
+ */
+ errno = 0;
+ size = strtoimax (str, &end, 10);
(1) A further improvement here (likely best done in a separate patch)
could be to change the type of "size" to "intmax_t", from
"int64_t".
That way, the assignment will be safe even theoretically, *and* the
overflow check at the bottom of the function (with the division &
comparison of the quotient against INT_MAX) will work just the same.
+ if (str == end) {
+ *error = "could not parse size string";
+ *pstr = str;
+ return -1;
+ }
+ if (size < 0) {
+ *error = "size cannot be negative";
+ *pstr = str;
+ return -1;
+ }
+ if (errno) {
+ *error = "size exceeds maximum value";
+ *pstr = str;
+ return -1;
+ }
+
+ switch (*end) {
+ /* No suffix */
+ case '\0':
+ end--; /* Safe, since we already filtered out empty string */
+ break;
+
+ /* Powers of 1024 */
+ case 'e': case 'E':
+ scale *= 1024;
+ /* fallthru */
+ case 'p': case 'P':
+ scale *= 1024;
+ /* fallthru */
+ case 't': case 'T':
+ scale *= 1024;
+ /* fallthru */
+ case 'g': case 'G':
+ scale *= 1024;
+ /* fallthru */
+ case 'm': case 'M':
+ scale *= 1024;
+ /* fallthru */
+ case 'k': case 'K':
+ scale *= 1024;
+ /* fallthru */
+ case 'b': case 'B':
+ break;
+
+ /* "sectors", ie. units of 512 bytes, even if that's not the real
+ * sector size
+ */
+ case 's': case 'S':
+ scale = 512;
+ break;
+
+ default:
+ *error = "could not parse size: unknown suffix";
+ *pstr = end;
+ return -1;
+ }
+
+ /* XXX Maybe we should support 'MiB' as a synonym for 'M'; and
'MB'
+ * for powers of 1000, for similarity to GNU tools. But for now,
+ * anything beyond 'M' is dropped.
+ */
+ if (end[1]) {
+ *error = "could not parse size: unknown suffix";
+ *pstr = end;
+ return -1;
+ }
+
+ if (INT64_MAX / scale < size) {
+ *error = "could not parse size: size * scale overflows";
+ *pstr = str;
+ return -1;
+ }
+
+ return size * scale;
+}
+
+#endif /* NBDKIT_HUMAN_SIZE_H */
diff --git a/common/include/test-human-size.c b/common/include/test-human-size.c
new file mode 100644
index 000000000..e8cbe7f41
--- /dev/null
+++ b/common/include/test-human-size.c
@@ -0,0 +1,133 @@
+/* nbdkit
+ * Copyright Red Hat
+ *
+ * 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.
+ */
+
+#include <config.h>
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <stdbool.h>
+#include <stdint.h>
+
+#include "array-size.h"
+#include "human-size.h"
+
+int
+main (void)
+{
+ size_t i;
+ bool pass = true;
+ struct pair {
+ const char *str;
+ int64_t res;
+ } pairs[] = {
+ /* Bogus strings */
+ { "", -1 },
+ { "0x0", -1 },
+ { "garbage", -1 },
+ { "0garbage", -1 },
+ { "8E", -1 },
+ { "8192P", -1 },
+
+ /* Strings leading to overflow */
+ { "9223372036854775808", -1 }, /* INT_MAX + 1 */
+ { "18446744073709551614", -1 }, /* UINT64_MAX - 1 */
+ { "18446744073709551615", -1 }, /* UINT64_MAX */
+ { "18446744073709551616", -1 }, /* UINT64_MAX + 1 */
+ { "999999999999999999999999", -1 },
+
+ /* Strings representing negative values */
+ { "-1", -1 },
+ { "-2", -1 },
+ { "-9223372036854775809", -1 }, /* INT64_MIN - 1 */
+ { "-9223372036854775808", -1 }, /* INT64_MIN */
+ { "-9223372036854775807", -1 }, /* INT64_MIN + 1 */
+ { "-18446744073709551616", -1 }, /* -UINT64_MAX - 1 */
+ { "-18446744073709551615", -1 }, /* -UINT64_MAX */
+ { "-18446744073709551614", -1 }, /* -UINT64_MAX + 1 */
+
+ /* Strings we may want to support in the future */
+ { "M", -1 },
+ { "1MB", -1 },
+ { "1MiB", -1 },
+ { "1.5M", -1 },
+
+ /* Valid strings */
+ { "-0", 0 },
+ { "0", 0 },
+ { "+0", 0 },
+ { " 08", 8 },
+ { "1", 1 },
+ { "+1", 1 },
+ { "1234567890", 1234567890 },
+ { "+1234567890", 1234567890 },
+ { "9223372036854775807", INT64_MAX },
+ { "1s", 512 },
+ { "2S", 1024 },
+ { "1b", 1 },
+ { "1B", 1 },
+ { "1k", 1024 },
+ { "1K", 1024 },
+ { "1m", 1024 * 1024 },
+ { "1M", 1024 * 1024 },
+ { "+1M", 1024 * 1024 },
+ { "1g", 1024 * 1024 * 1024 },
+ { "1G", 1024 * 1024 * 1024 },
+ { "1t", 1024LL * 1024 * 1024 * 1024 },
+ { "1T", 1024LL * 1024 * 1024 * 1024 },
+ { "1p", 1024LL * 1024 * 1024 * 1024 * 1024 },
+ { "1P", 1024LL * 1024 * 1024 * 1024 * 1024 },
+ { "8191p", 1024LL * 1024 * 1024 * 1024 * 1024 * 8191 },
+ { "1e", 1024LL * 1024 * 1024 * 1024 * 1024 * 1024 },
+ { "1E", 1024LL * 1024 * 1024 * 1024 * 1024 * 1024 },
+ };
+
+ for (i = 0; i < ARRAY_SIZE (pairs); i++) {
+ const char *error = NULL, *pstr = NULL;
+ int64_t r;
+
+ r = human_size_parse (pairs[i].str, &error, &pstr);
+ if (r != pairs[i].res) {
+ fprintf (stderr,
+ "Wrong parse for %s, got %" PRId64 ", expected %"
PRId64 "\n",
+ pairs[i].str, r, pairs[i].res);
+ pass = false;
+ }
+ if (r == -1) {
+ if (error == NULL || pstr == NULL) {
+ fprintf (stderr, "Wrong error message handling for %s\n",
pairs[i].str);
+ pass = false;
+ }
+ }
+ }
+
+ exit (pass ? EXIT_SUCCESS : EXIT_FAILURE);
+}
(2) I don't like that we're repeating the test cases here, from
test_nbdkit_parse_size() [server/test-public.c].
Originally I intended to ask "why not just *move* that code as well",
but I think I see the point...
Namely, in test_nbdkit_parse_size(), we still need to test
nbdkit_error() -- via "error_flagged" --, and nbdkit_error() remains
unique to test_nbdkit_parse_size(), after factoring out
human_size_parse(). And so, for triggering the errors, we need to keep
the same test cases.
... Would it be possible to move the "pairs" array into a separate C
file under "common"? (Not necessarily under "common/include".)
We'd need
a new header file (for defining the "pair" type, for declaring the
"pairs" array, and for declaring the "num_pairs" constant, which
would
have to be a global variable then.)
If that's too difficult or intrusive, then please at least
cross-reference each source file from the other, in new comments, so
that whenever we update one of them, we don't forget the other.
(3) Calling "exit" at the end is a bit awkward to me. Correct, but
"return" would work just as fine.
With the cross-refs added:
Reviewed-by: Laszlo Ersek <lersek(a)redhat.com>
Laszlo
diff --git a/server/public.c b/server/public.c
index 705ac3a47..a1ba603d4 100644
--- a/server/public.c
+++ b/server/public.c
@@ -76,6 +76,7 @@
#include "ascii-string.h"
#include "get_current_dir_name.h"
#include "getline.h"
+#include "human-size.h"
#include "poll.h"
#include "realpath.h"
#include "strndup.h"
@@ -343,83 +344,16 @@ nbdkit_parse_uint64_t (const char *what, const char *str, uint64_t
*rp)
NBDKIT_DLL_PUBLIC int64_t
nbdkit_parse_size (const char *str)
{
+ const char *error, *pstr;
int64_t size;
- char *end;
- uint64_t scale = 1;
- /* Disk sizes cannot usefully exceed off_t (which is signed) and
- * cannot be negative. */
- /* XXX Should we also parse things like '1.5M'? */
- /* XXX Should we allow hex? If so, hex cannot use scaling suffixes,
- * because some of them are valid hex digits */
- errno = 0;
- size = strtoimax (str, &end, 10);
- if (str == end) {
- nbdkit_error ("could not parse size string (%s)", str);
- return -1;
- }
- if (size < 0) {
- nbdkit_error ("size cannot be negative (%s)", str);
- return -1;
- }
- if (errno) {
- nbdkit_error ("size (%s) exceeds maximum value", str);
- return -1;
- }
-
- switch (*end) {
- /* No suffix */
- case '\0':
- end--; /* Safe, since we already filtered out empty string */
- break;
-
- /* Powers of 1024 */
- case 'e': case 'E':
- scale *= 1024;
- /* fallthru */
- case 'p': case 'P':
- scale *= 1024;
- /* fallthru */
- case 't': case 'T':
- scale *= 1024;
- /* fallthru */
- case 'g': case 'G':
- scale *= 1024;
- /* fallthru */
- case 'm': case 'M':
- scale *= 1024;
- /* fallthru */
- case 'k': case 'K':
- scale *= 1024;
- /* fallthru */
- case 'b': case 'B':
- break;
-
- /* "sectors", ie. units of 512 bytes, even if that's not the real
- * sector size */
- case 's': case 'S':
- scale = 512;
- break;
-
- default:
- nbdkit_error ("could not parse size: unknown suffix '%s'", end);
- return -1;
- }
-
- /* XXX Maybe we should support 'MiB' as a synonym for 'M'; and
'MB'
- * for powers of 1000, for similarity to GNU tools. But for now,
- * anything beyond 'M' is dropped. */
- if (end[1]) {
- nbdkit_error ("could not parse size: unknown suffix '%s'", end);
- return -1;
- }
-
- if (INT64_MAX / scale < size) {
- nbdkit_error ("overflow computing size (%s)", str);
+ size = human_size_parse (str, &error, &pstr);
+ if (size == -1) {
+ nbdkit_error ("%s: %s", error, pstr);
return -1;
}
- return size * scale;
+ return size;
}
NBDKIT_DLL_PUBLIC int
diff --git a/.gitignore b/.gitignore
index 49af3998f..04fdcd723 100644
--- a/.gitignore
+++ b/.gitignore
@@ -40,6 +40,7 @@ plugins/*/*.3
/common/include/test-ascii-string
/common/include/test-byte-swapping
/common/include/test-checked-overflow
+/common/include/test-human-size
/common/include/test-isaligned
/common/include/test-ispowerof2
/common/include/test-iszero
5:09 a.m.
On Mon, Sep 04, 2023 at 09:52:53AM +0200, Laszlo Ersek wrote:
On 9/3/23 17:22, Richard W.M. Jones wrote:
> This is broadly simple code motion, intended so that we can reuse the
> same code in libnbd.
> ---
> common/include/Makefile.am | 6 ++
> common/include/human-size.h | 137 +++++++++++++++++++++++++++++++
> common/include/test-human-size.c | 133 ++++++++++++++++++++++++++++++
> server/public.c | 78 ++----------------
> .gitignore | 1 +
> 5 files changed, 283 insertions(+), 72 deletions(-)
>
> diff --git a/common/include/Makefile.am b/common/include/Makefile.am
> index 3162e92c2..8e4de04f3 100644
> --- a/common/include/Makefile.am
> +++ b/common/include/Makefile.am
> @@ -42,6 +42,7 @@ EXTRA_DIST = \
> checked-overflow.h \
> compiler-macros.h \
> hexdigit.h \
> + human-size.h \
> isaligned.h \
> ispowerof2.h \
> iszero.h \
> @@ -63,6 +64,7 @@ TESTS = \
> test-ascii-string \
> test-byte-swapping \
> test-checked-overflow \
> + test-human-size \
> test-isaligned \
> test-ispowerof2 \
> test-iszero \
> @@ -93,6 +95,10 @@ test_checked_overflow_SOURCES = test-checked-overflow.c
checked-overflow.h
> test_checked_overflow_CPPFLAGS = -I$(srcdir)
> test_checked_overflow_CFLAGS = $(WARNINGS_CFLAGS)
>
> +test_human_size_SOURCES = test-human-size.c human-size.h
> +test_human_size_CPPFLAGS = -I$(srcdir)
> +test_human_size_CFLAGS = $(WARNINGS_CFLAGS)
> +
> test_isaligned_SOURCES = test-isaligned.c isaligned.h
> test_isaligned_CPPFLAGS = -I$(srcdir)
> test_isaligned_CFLAGS = $(WARNINGS_CFLAGS)
> diff --git a/common/include/human-size.h b/common/include/human-size.h
> new file mode 100644
> index 000000000..788dbd7ba
> --- /dev/null
> +++ b/common/include/human-size.h
> @@ -0,0 +1,137 @@
> +/* nbdkit
> + * Copyright Red Hat
> + *
> + * 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_HUMAN_SIZE_H
> +#define NBDKIT_HUMAN_SIZE_H
> +
> +#include <stdint.h>
> +#include <inttypes.h>
> +#include <errno.h>
> +
> +/* Attempt to parse a string with a possible scaling suffix, such as
> + * "2M". Disk sizes cannot usefully exceed off_t (which is signed)
> + * and cannot be negative.
> + *
> + * On error, returns -1 and sets *error and *pstr. You can form a
> + * final error message by appending "<error>: <pstr>".
> + */
> +static inline int64_t
> +human_size_parse (const char *str,
> + const char **error, const char **pstr)
> +{
> + int64_t size;
> + char *end;
> + uint64_t scale = 1;
> +
> + /* XXX Should we also parse things like '1.5M'? */
> + /* XXX Should we allow hex? If so, hex cannot use scaling suffixes,
> + * because some of them are valid hex digits.
> + */
> + errno = 0;
> + size = strtoimax (str, &end, 10);
(1) A further improvement here (likely best done in a separate patch)
could be to change the type of "size" to "intmax_t", from
"int64_t".
That way, the assignment will be safe even theoretically, *and* the
overflow check at the bottom of the function (with the division &
comparison of the quotient against INT_MAX) will work just the same.
I'm always very unsure how this all works. In particular I seem to
recall that intmax_t is no longer really the maximum possible int
(because of int128) and so it's always 64 bit on platforms we care
about. Can Eric comment?
> + if (str == end) {
> + *error = "could not parse size string";
> + *pstr = str;
> + return -1;
> + }
> + if (size < 0) {
> + *error = "size cannot be negative";
> + *pstr = str;
> + return -1;
> + }
> + if (errno) {
> + *error = "size exceeds maximum value";
> + *pstr = str;
> + return -1;
> + }
> +
> + switch (*end) {
> + /* No suffix */
> + case '\0':
> + end--; /* Safe, since we already filtered out empty string */
> + break;
> +
> + /* Powers of 1024 */
> + case 'e': case 'E':
> + scale *= 1024;
> + /* fallthru */
> + case 'p': case 'P':
> + scale *= 1024;
> + /* fallthru */
> + case 't': case 'T':
> + scale *= 1024;
> + /* fallthru */
> + case 'g': case 'G':
> + scale *= 1024;
> + /* fallthru */
> + case 'm': case 'M':
> + scale *= 1024;
> + /* fallthru */
> + case 'k': case 'K':
> + scale *= 1024;
> + /* fallthru */
> + case 'b': case 'B':
> + break;
> +
> + /* "sectors", ie. units of 512 bytes, even if that's not the
real
> + * sector size
> + */
> + case 's': case 'S':
> + scale = 512;
> + break;
> +
> + default:
> + *error = "could not parse size: unknown suffix";
> + *pstr = end;
> + return -1;
> + }
> +
> + /* XXX Maybe we should support 'MiB' as a synonym for 'M'; and
'MB'
> + * for powers of 1000, for similarity to GNU tools. But for now,
> + * anything beyond 'M' is dropped.
> + */
> + if (end[1]) {
> + *error = "could not parse size: unknown suffix";
> + *pstr = end;
> + return -1;
> + }
> +
> + if (INT64_MAX / scale < size) {
> + *error = "could not parse size: size * scale overflows";
> + *pstr = str;
> + return -1;
> + }
> +
> + return size * scale;
> +}
> +
> +#endif /* NBDKIT_HUMAN_SIZE_H */
> diff --git a/common/include/test-human-size.c b/common/include/test-human-size.c
> new file mode 100644
> index 000000000..e8cbe7f41
> --- /dev/null
> +++ b/common/include/test-human-size.c
> @@ -0,0 +1,133 @@
> +/* nbdkit
> + * Copyright Red Hat
> + *
> + * 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.
> + */
> +
> +#include <config.h>
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <stdbool.h>
> +#include <stdint.h>
> +
> +#include "array-size.h"
> +#include "human-size.h"
> +
> +int
> +main (void)
> +{
> + size_t i;
> + bool pass = true;
> + struct pair {
> + const char *str;
> + int64_t res;
> + } pairs[] = {
> + /* Bogus strings */
> + { "", -1 },
> + { "0x0", -1 },
> + { "garbage", -1 },
> + { "0garbage", -1 },
> + { "8E", -1 },
> + { "8192P", -1 },
> +
> + /* Strings leading to overflow */
> + { "9223372036854775808", -1 }, /* INT_MAX + 1 */
> + { "18446744073709551614", -1 }, /* UINT64_MAX - 1 */
> + { "18446744073709551615", -1 }, /* UINT64_MAX */
> + { "18446744073709551616", -1 }, /* UINT64_MAX + 1 */
> + { "999999999999999999999999", -1 },
> +
> + /* Strings representing negative values */
> + { "-1", -1 },
> + { "-2", -1 },
> + { "-9223372036854775809", -1 }, /* INT64_MIN - 1 */
> + { "-9223372036854775808", -1 }, /* INT64_MIN */
> + { "-9223372036854775807", -1 }, /* INT64_MIN + 1 */
> + { "-18446744073709551616", -1 }, /* -UINT64_MAX - 1 */
> + { "-18446744073709551615", -1 }, /* -UINT64_MAX */
> + { "-18446744073709551614", -1 }, /* -UINT64_MAX + 1 */
> +
> + /* Strings we may want to support in the future */
> + { "M", -1 },
> + { "1MB", -1 },
> + { "1MiB", -1 },
> + { "1.5M", -1 },
> +
> + /* Valid strings */
> + { "-0", 0 },
> + { "0", 0 },
> + { "+0", 0 },
> + { " 08", 8 },
> + { "1", 1 },
> + { "+1", 1 },
> + { "1234567890", 1234567890 },
> + { "+1234567890", 1234567890 },
> + { "9223372036854775807", INT64_MAX },
> + { "1s", 512 },
> + { "2S", 1024 },
> + { "1b", 1 },
> + { "1B", 1 },
> + { "1k", 1024 },
> + { "1K", 1024 },
> + { "1m", 1024 * 1024 },
> + { "1M", 1024 * 1024 },
> + { "+1M", 1024 * 1024 },
> + { "1g", 1024 * 1024 * 1024 },
> + { "1G", 1024 * 1024 * 1024 },
> + { "1t", 1024LL * 1024 * 1024 * 1024 },
> + { "1T", 1024LL * 1024 * 1024 * 1024 },
> + { "1p", 1024LL * 1024 * 1024 * 1024 * 1024 },
> + { "1P", 1024LL * 1024 * 1024 * 1024 * 1024 },
> + { "8191p", 1024LL * 1024 * 1024 * 1024 * 1024 * 8191 },
> + { "1e", 1024LL * 1024 * 1024 * 1024 * 1024 * 1024 },
> + { "1E", 1024LL * 1024 * 1024 * 1024 * 1024 * 1024 },
> + };
> +
> + for (i = 0; i < ARRAY_SIZE (pairs); i++) {
> + const char *error = NULL, *pstr = NULL;
> + int64_t r;
> +
> + r = human_size_parse (pairs[i].str, &error, &pstr);
> + if (r != pairs[i].res) {
> + fprintf (stderr,
> + "Wrong parse for %s, got %" PRId64 ", expected
%" PRId64 "\n",
> + pairs[i].str, r, pairs[i].res);
> + pass = false;
> + }
> + if (r == -1) {
> + if (error == NULL || pstr == NULL) {
> + fprintf (stderr, "Wrong error message handling for %s\n",
pairs[i].str);
> + pass = false;
> + }
> + }
> + }
> +
> + exit (pass ? EXIT_SUCCESS : EXIT_FAILURE);
> +}
(2) I don't like that we're repeating the test cases here, from
test_nbdkit_parse_size() [server/test-public.c].
Originally I intended to ask "why not just *move* that code as well",
but I think I see the point...
Namely, in test_nbdkit_parse_size(), we still need to test
nbdkit_error() -- via "error_flagged" --, and nbdkit_error() remains
unique to test_nbdkit_parse_size(), after factoring out
human_size_parse(). And so, for triggering the errors, we need to keep
the same test cases.
... Would it be possible to move the "pairs" array into a separate C
file under "common"? (Not necessarily under "common/include".)
We'd need
a new header file (for defining the "pair" type, for declaring the
"pairs" array, and for declaring the "num_pairs" constant, which
would
have to be a global variable then.)
If that's too difficult or intrusive, then please at least
cross-reference each source file from the other, in new comments, so
that whenever we update one of them, we don't forget the other.
Actually it was easy enough to make this into a common file
("common/include/human-size-test-cases.h") in the final version.
(3) Calling "exit" at the end is a bit awkward to me.
Correct, but
"return" would work just as fine.
With the cross-refs added:
Reviewed-by: Laszlo Ersek <lersek(a)redhat.com>
Thanks,
I pushed this as commit 29ce734ad.
Rich.
Laszlo
> diff --git a/server/public.c b/server/public.c
> index 705ac3a47..a1ba603d4 100644
> --- a/server/public.c
> +++ b/server/public.c
> @@ -76,6 +76,7 @@
> #include "ascii-string.h"
> #include "get_current_dir_name.h"
> #include "getline.h"
> +#include "human-size.h"
> #include "poll.h"
> #include "realpath.h"
> #include "strndup.h"
> @@ -343,83 +344,16 @@ nbdkit_parse_uint64_t (const char *what, const char *str,
uint64_t *rp)
> NBDKIT_DLL_PUBLIC int64_t
> nbdkit_parse_size (const char *str)
> {
> + const char *error, *pstr;
> int64_t size;
> - char *end;
> - uint64_t scale = 1;
>
> - /* Disk sizes cannot usefully exceed off_t (which is signed) and
> - * cannot be negative. */
> - /* XXX Should we also parse things like '1.5M'? */
> - /* XXX Should we allow hex? If so, hex cannot use scaling suffixes,
> - * because some of them are valid hex digits */
> - errno = 0;
> - size = strtoimax (str, &end, 10);
> - if (str == end) {
> - nbdkit_error ("could not parse size string (%s)", str);
> - return -1;
> - }
> - if (size < 0) {
> - nbdkit_error ("size cannot be negative (%s)", str);
> - return -1;
> - }
> - if (errno) {
> - nbdkit_error ("size (%s) exceeds maximum value", str);
> - return -1;
> - }
> -
> - switch (*end) {
> - /* No suffix */
> - case '\0':
> - end--; /* Safe, since we already filtered out empty string */
> - break;
> -
> - /* Powers of 1024 */
> - case 'e': case 'E':
> - scale *= 1024;
> - /* fallthru */
> - case 'p': case 'P':
> - scale *= 1024;
> - /* fallthru */
> - case 't': case 'T':
> - scale *= 1024;
> - /* fallthru */
> - case 'g': case 'G':
> - scale *= 1024;
> - /* fallthru */
> - case 'm': case 'M':
> - scale *= 1024;
> - /* fallthru */
> - case 'k': case 'K':
> - scale *= 1024;
> - /* fallthru */
> - case 'b': case 'B':
> - break;
> -
> - /* "sectors", ie. units of 512 bytes, even if that's not the
real
> - * sector size */
> - case 's': case 'S':
> - scale = 512;
> - break;
> -
> - default:
> - nbdkit_error ("could not parse size: unknown suffix '%s'",
end);
> - return -1;
> - }
> -
> - /* XXX Maybe we should support 'MiB' as a synonym for 'M'; and
'MB'
> - * for powers of 1000, for similarity to GNU tools. But for now,
> - * anything beyond 'M' is dropped. */
> - if (end[1]) {
> - nbdkit_error ("could not parse size: unknown suffix '%s'",
end);
> - return -1;
> - }
> -
> - if (INT64_MAX / scale < size) {
> - nbdkit_error ("overflow computing size (%s)", str);
> + size = human_size_parse (str, &error, &pstr);
> + if (size == -1) {
> + nbdkit_error ("%s: %s", error, pstr);
> return -1;
> }
>
> - return size * scale;
> + return size;
> }
>
> NBDKIT_DLL_PUBLIC int
> diff --git a/.gitignore b/.gitignore
> index 49af3998f..04fdcd723 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -40,6 +40,7 @@ plugins/*/*.3
> /common/include/test-ascii-string
> /common/include/test-byte-swapping
> /common/include/test-checked-overflow
> +/common/include/test-human-size
> /common/include/test-isaligned
> /common/include/test-ispowerof2
> /common/include/test-iszero
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines. Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v
9:57 a.m.
On Tue, Sep 05, 2023 at 11:09:02AM +0100, Richard W.M. Jones wrote:
> > +static inline int64_t
> > +human_size_parse (const char *str,
> > + const char **error, const char **pstr)
> > +{
> > + int64_t size;
> > + char *end;
> > + uint64_t scale = 1;
> > +
> > + /* XXX Should we also parse things like '1.5M'? */
> > + /* XXX Should we allow hex? If so, hex cannot use scaling suffixes,
> > + * because some of them are valid hex digits.
> > + */
> > + errno = 0;
> > + size = strtoimax (str, &end, 10);
>
> (1) A further improvement here (likely best done in a separate patch)
> could be to change the type of "size" to "intmax_t", from
"int64_t".
> That way, the assignment will be safe even theoretically, *and* the
> overflow check at the bottom of the function (with the division &
> comparison of the quotient against INT_MAX) will work just the same.
I'm always very unsure how this all works. In particular I seem to
recall that intmax_t is no longer really the maximum possible int
(because of int128) and so it's always 64 bit on platforms we care
about. Can Eric comment?
intmax_t was supposed to be whatever the compiler supports as its
largest integer type; but you are right that when gcc added
__int128_t, they did NOT change intmax_t at the time (arguably by
using weasel-words that as an implementation-defined type, it was not
an integer type merely because you can't write integer literals of
that type, even though it behaves integral in every other aspect).
We're kind of in a frozen state where making intmax_t larger than 64
bits will break more programs than expected because it has ABI
implications:
https://stackoverflow.com/questions/21265462/why-in-g-stdintmax-t-is-not-...
My personal preference is to avoid intmax_t, as it has too much
baggage (the risk of future widening, vs. NOT being the widest type
after all), similar to how size_t already has baggage. In short,
having something that is not platform specific is easier to reason
about (for the same way that using size_t gives me more grief than
directly using int32_t or int64_t; even though size_t is such a
naturally occuring type, the fact that it is not uniform width makes
it trickier to work with).
> > +
> > + if (INT64_MAX / scale < size) {
> > + *error = "could not parse size: size * scale overflows";
> > + *pstr = str;
> > + return -1;
> > + }
And thus I prefer that this comparison stay pegged to INT64_MAX, and
not INT_MAX.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization: qemu.org | libguestfs.org
3:58 a.m.
On 9/5/23 16:57, Eric Blake wrote:
On Tue, Sep 05, 2023 at 11:09:02AM +0100, Richard W.M. Jones wrote:
>>> +static inline int64_t
>>> +human_size_parse (const char *str,
>>> + const char **error, const char **pstr)
>>> +{
>>> + int64_t size;
>>> + char *end;
>>> + uint64_t scale = 1;
>>> +
>>> + /* XXX Should we also parse things like '1.5M'? */
>>> + /* XXX Should we allow hex? If so, hex cannot use scaling suffixes,
>>> + * because some of them are valid hex digits.
>>> + */
>>> + errno = 0;
>>> + size = strtoimax (str, &end, 10);
>>
>> (1) A further improvement here (likely best done in a separate patch)
>> could be to change the type of "size" to "intmax_t", from
"int64_t".
>> That way, the assignment will be safe even theoretically, *and* the
>> overflow check at the bottom of the function (with the division &
>> comparison of the quotient against INT_MAX) will work just the same.
>
> I'm always very unsure how this all works. In particular I seem to
> recall that intmax_t is no longer really the maximum possible int
> (because of int128) and so it's always 64 bit on platforms we care
> about. Can Eric comment?
intmax_t was supposed to be whatever the compiler supports as its
largest integer type; but you are right that when gcc added
__int128_t, they did NOT change intmax_t at the time (arguably by
using weasel-words that as an implementation-defined type, it was not
an integer type merely because you can't write integer literals of
that type, even though it behaves integral in every other aspect).
We're kind of in a frozen state where making intmax_t larger than 64
bits will break more programs than expected because it has ABI
implications:
https://stackoverflow.com/questions/21265462/why-in-g-stdintmax-t-is-not-...
The fact (which I just learned today) that ABIs are unable to keep up
with intmax_t is a disaster, and entirely defeats the purpose of intmax_t.
My personal preference is to avoid intmax_t, as it has too much
baggage (the risk of future widening, vs. NOT being the widest type
after all), similar to how size_t already has baggage. In short,
having something that is not platform specific is easier to reason
about (for the same way that using size_t gives me more grief than
directly using int32_t or int64_t; even though size_t is such a
naturally occuring type, the fact that it is not uniform width makes
it trickier to work with).
Very painful disconnect between platforms (which prefer fixed size
integers) and the C standard (which defines all the rules with standard
integer types).
>>> +
>>> + if (INT64_MAX / scale < size) {
>>> + *error = "could not parse size: size * scale overflows";
>>> + *pstr = str;
>>> + return -1;
>>> + }
And thus I prefer that this comparison stay pegged to INT64_MAX, and
not INT_MAX.
Side comment: I never suggested INT64_MAX be replaced with INTMAX_MAX
here, and that was deliberate on my part. I only suggested changing the
type of "size" from "int64_t" to "intmax_t", so that
"size"'s type would
literally match the retval type of strtoimax().
And after such a type change, the expression (INT64_MAX / scale < size)
would remain exactly right:
- All the participating values remain nonnegative, so whatever usual
arithmetic conversions are taken, the values will never change.
- Keeping the INT64_MAX limit continues to make sure that the final
(size * scale) product, although its type *might* change, will produce
the same safe value, for returning through an int64_t. (We'd not want
the retval type to change!)
... Ahhh! I now see where the confusion comes from. My mistake! I wrote:
That way, the assignment will be safe even theoretically, *and* the
overflow check at the bottom of the function (with the division &
comparison of the quotient against INT_MAX) will work just the same.
That's a terrible typo! I meant to write INT64_MAX!
Laszlo
443
days inactive
446
days old
5 comments
3 participants
participants (3)
-
Eric Blake -
Laszlo Ersek -
Richard W.M. Jones