The existing implementation admitted that it was not very robust
in the face of garbage input; tighten things up by reimplementing
the function, and add testsuite coverage to ensure further tweaks
do not break things.
The testsuite additions were interesting - we didn't have any easy
way to link against a subset of the src/ files (all previous uses
of the util functions have been through dynamic .so libraries,
rather than standalone binaries).
Signed-off-by: Eric Blake <ebl...@redhat.com>
---
RFC, because I don't know if I like my testsuite additions; automake
in particular was rather noisy about it:
tests/Makefile.am:99: warning: source file '$(top_srcdir)/src/utils.c' is in a
subdirectory,
tests/Makefile.am:99: but option 'subdir-objects' is disabled
automake-1.15: warning: possible forward-incompatibility.
automake-1.15: At least a source file is in a subdirectory, but the
'subdir-objects'
automake-1.15: automake option hasn't been enabled. For now, the corresponding
output
automake-1.15: object file(s) will be placed in the top-level directory.
However,
automake-1.15: this behaviour will change in future Automake versions: they will
automake-1.15: unconditionally cause object files to be placed in the same
subdirectory
automake-1.15: of the corresponding sources.
automake-1.15: You are advised to start using 'subdir-objects' option
throughout your
automake-1.15: project, to avoid future incompatibilities.
Maybe there's a smarter way to do this?
Or, I could just commit the src/utils.c change, and leave out the
testsuite additions.
---
src/utils.c | 95 ++++++++++++++++++++++++++---------------
tests/Makefile.am | 11 +++++
tests/test-utils.c | 121 +++++++++++++++++++++++++++++++++++++++++++++++++++++
.gitignore | 1 +
4 files changed, 195 insertions(+), 33 deletions(-)
create mode 100644 tests/test-utils.c
diff --git a/src/utils.c b/src/utils.c
index 5663043..6e04bc8 100644
--- a/src/utils.c
+++ b/src/utils.c
@@ -1,5 +1,5 @@
/* nbdkit
- * Copyright (C) 2013 Red Hat Inc.
+ * Copyright (C) 2013-2018 Red Hat Inc.
* All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
@@ -80,49 +80,78 @@ nbdkit_absolute_path (const char *path)
return ret;
}
-/* XXX Multiple problems with this function. Really we should use the
- * 'human*' functions from gnulib.
+/* Parse a string with possible scaling suffix, or return -1 after reporting
+ * the error.
*/
int64_t
nbdkit_parse_size (const char *str)
{
uint64_t size;
- char t;
+ char *end;
+ uint64_t scale = 1;
- if (sscanf (str, "%" SCNu64 "%c", &size, &t) == 2) {
- switch (t) {
- case 'b': case 'B':
- return (int64_t) size;
- case 'k': case 'K':
- return (int64_t) size * 1024;
- case 'm': case 'M':
- return (int64_t) size * 1024 * 1024;
- case 'g': case 'G':
- return (int64_t) size * 1024 * 1024 * 1024;
- case 't': case 'T':
- return (int64_t) size * 1024 * 1024 * 1024 * 1024;
- case 'p': case 'P':
- return (int64_t) size * 1024 * 1024 * 1024 * 1024 * 1024;
- case 'e': case 'E':
- return (int64_t) size * 1024 * 1024 * 1024 * 1024 * 1024 * 1024;
+ /* Scan unsigned, but range check later that result fits in signed. */
+ /* 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 = strtoumax (str, &end, 10);
+ if (errno || str == end) {
+ nbdkit_error ("could not parse size string (%s)", str);
+ return -1;
+ }
+ switch (*end) {
+ 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;
- case 's': case 'S': /* "sectors", ie. units of 512 bytes,
- * even if that's not the real sector size
- */
- return (int64_t) size * 512;
+ case 's': case 'S': /* "sectors", ie. units of 512 bytes,
+ * even if that's not the real sector size
+ */
+ scale = 512;
+ break;
- default:
- nbdkit_error ("could not parse size: unknown specifier '%c'", t);
- return -1;
- }
+ default:
+ nbdkit_error ("could not parse size: unknown suffix '%s'", end);
+ return -1;
}
- /* bytes */
- if (sscanf (str, "%" SCNu64, &size) == 1)
- return (int64_t) size;
+ /* XXX Maybe we should support 'MiB' as 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);
+ return -1;
+ }
- nbdkit_error ("could not parse size string (%s)", str);
- return -1;
+ return size * scale;
}
/* Read a password from configuration value. */
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 1e32cb6..baae6a1 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -90,8 +90,19 @@ TESTS_ENVIRONMENT = PATH=$(abs_top_builddir):$(PATH)
TESTS = \
test-help.sh \
test-version.sh \
+ test-utils \
test-dump-config.sh
+check_PROGRAMS += \
+ test-utils
+
+test_utils_SOURCES = test-utils.c \
+ $(top_srcdir)/src/utils.c \
+ $(top_srcdir)/src/cleanup.c \
+ $(top_srcdir)/include/nbdkit-plugin.h
+test_utils_CPPFLAGS = -I$(top_srcdir)/include
+test_utils_CFLAGS = $(WARNINGS_CFLAGS)
+
if HAVE_PLUGINS
TESTS += \
diff --git a/tests/test-utils.c b/tests/test-utils.c
new file mode 100644
index 0000000..af08416
--- /dev/null
+++ b/tests/test-utils.c
@@ -0,0 +1,121 @@
+/* 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.
+ */
+
+#include <config.h>
+
+#include <stdio.h>
+#include <inttypes.h>
+#include <stdbool.h>
+
+#include <nbdkit-plugin.h>
+
+static bool error_flagged;
+
+/* Stub for linking against minimal source files */
+void
+nbdkit_error (const char *fs, ...)
+{
+ error_flagged = true;
+}
+
+static bool
+test_nbdkit_parse_size (void)
+{
+ bool pass = true;
+ struct pair {
+ const char *str;
+ int64_t res;
+ } pairs[] = {
+ /* Bogus strings */
+ { "", -1 },
+ { "0x0", -1 },
+ { "garbage", -1 },
+ { "0garbage", -1 },
+ { "M", -1 },
+ { "-1", -1 },
+ { "-2", -1 },
+ { "9223372036854775808", -1 },
+ { "-9223372036854775808", -1 },
+ { "8E", -1 },
+ { "8192P", -1 },
+
+ /* Valid strings */
+ { "0", 0 },
+ { " 08", 8 },
+ { "9223372036854775807", INT64_MAX },
+ { "1s", 512 },
+ { "2S", 1024 },
+ { "1b", 1 },
+ { "1B", 1 },
+ { "1k", 1024 },
+ { "1K", 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 },
+ { "1e", 1024LL * 1024 * 1024 * 1024 * 1024 * 1024 },
+ { "1E", 1024LL * 1024 * 1024 * 1024 * 1024 * 1024 },
+ };
+
+ for (size_t i = 0; i < sizeof pairs / sizeof pairs[0]; i++) {
+ int64_t r;
+
+ error_flagged = false;
+ r = nbdkit_parse_size (pairs[i].str);
+ 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) != error_flagged) {
+ fprintf (stderr, "Wrong error message handling for %s\n", pairs[i].str);
+ pass = false;
+ }
+ }
+
+ return pass;
+}
+
+int
+main (int argc, char *argv[])
+{
+ bool pass = true;
+ pass &= test_nbdkit_parse_size ();
+ /* XXX tests nbdkit_absolute_path, nbdkit_read_password */
+ return pass ? 0 : 1;
+}
diff --git a/.gitignore b/.gitignore
index 09ed262..28215bc 100644
--- a/.gitignore
+++ b/.gitignore
@@ -72,5 +72,6 @@ Makefile.in
/tests/test-socket-activation
/tests/test-split
/tests/test-streaming
+/tests/test-utils
/tests/test-xz
/test-driver
--
2.14.3
_______________________________________________
Libguestfs mailing list
Libguestfs@redhat.com
https://www.redhat.com/mailman/listinfo/libguestfs
