[Qemu-devel] [PATCH 0/6] Fix strtosz users, clean up its implementation

[Qemu-devel] [PATCH 0/6] Fix strtosz users, clean up its implementation

[Markus Armbruster]

Markus Armbruster (6):
  cutils: Drop broken support for zero strtosz default_suffix
  vl: Tighten parsing of -numa's parameter mem
  vl: Tighten parsing of -m argument
  x86/cpuid: Tighten parsing of tsc_freq=FREQ
  qemu-img: Tighten parsing of size arguments
  cutils: Make strtosz & friends leave follow set to callers

 cutils.c            |   81 +++++++++++++++++----------------------------------
 qemu-img.c          |   10 ++++--
 target-i386/cpuid.c |    2 +-
 vl.c                |    9 +++--
 4 files changed, 39 insertions(+), 63 deletions(-)

-- 
1.7.6.4

[Qemu-devel] [PATCH 1/6] cutils: Drop broken support for zero strtosz default_suffix

[Markus Armbruster]

Commit 9f9b17a4's strtosz() defaults a missing suffix to 'M', except
it rejects fractions then (switch case 0).

When commit d8427002 introduced strtosz_suffix(), that changed:
fractions are no longer rejected, because we go to switch case 'M' on
missing suffix now.  Not mentioned in commit message, probably
unintentional.  Not worth changing back now.

Because case 0 is still around, you can get the old behavior by
passing a zero default_suffix to strtosz_suffix() or
strtosz_suffix_unit().  Undocumented and not used.  Drop.

Commit d8427002 also neglected to update the function comment.  Fix it
up.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 cutils.c |   17 ++++-------------
 1 files changed, 4 insertions(+), 13 deletions(-)

diff --git a/cutils.c b/cutils.c
index 5d995bc..55b0596 100644
--- a/cutils.c
+++ b/cutils.c
@@ -317,10 +317,9 @@ int fcntl_setfl(int fd, int flag)
 
 /*
  * Convert string to bytes, allowing either B/b for bytes, K/k for KB,
- * M/m for MB, G/g for GB or T/t for TB. Default without any postfix
- * is MB. End pointer will be returned in *end, if not NULL. A valid
- * value must be terminated by whitespace, ',' or '\0'. Return -1 on
- * error.
+ * M/m for MB, G/g for GB or T/t for TB. End pointer will be returned
+ * in *end, if not NULL. A valid value must be terminated by
+ * whitespace, ',' or '\0'. Return -1 on error.
  */
 int64_t strtosz_suffix_unit(const char *nptr, char **end,
                             const char default_suffix, int64_t unit)
@@ -349,11 +348,7 @@ int64_t strtosz_suffix_unit(const char *nptr, char **end,
     d = c;
     if (qemu_isspace(c) || c == '\0' || c == ',') {
         c = 0;
-        if (default_suffix) {
-            d = default_suffix;
-        } else {
-            d = c;
-        }
+        d = default_suffix;
     }
     switch (qemu_toupper(d)) {
     case STRTOSZ_DEFSUFFIX_B:
@@ -365,10 +360,6 @@ int64_t strtosz_suffix_unit(const char *nptr, char **end,
     case STRTOSZ_DEFSUFFIX_KB:
         mul = unit;
         break;
-    case 0:
-        if (mul_required) {
-            goto fail;
-        }
     case STRTOSZ_DEFSUFFIX_MB:
         mul = unit * unit;
         break;
-- 
1.7.6.4

[Qemu-devel] [PATCH 2/6] vl: Tighten parsing of -numa's parameter mem

[Markus Armbruster]

strtosz_suffix() fails unless the size is followed by 0, whitespace or
','.  Useless here, because we need to fail for any junk following the
size, even if it starts with whitespace or ','.  Check manually.

Things like

    -smp 4 -numa "node,mem=1024,cpus=0-1" -numa "node,mem=1024 cpus=2-3"

are now caught.  Before, the second -numa's argument was silently
interpreted as just "node,mem=1024".

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 vl.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/vl.c b/vl.c
index f5afed4..b9146cf 100644
--- a/vl.c
+++ b/vl.c
@@ -953,8 +953,8 @@ static void numa_add(const char *optarg)
             node_mem[nodenr] = 0;
         } else {
             int64_t sval;
-            sval = strtosz(option, NULL);
-            if (sval < 0) {
+            sval = strtosz(option, &endptr);
+            if (sval < 0 || *endptr) {
                 fprintf(stderr, "qemu: invalid numa mem size: %s\n", optarg);
                 exit(1);
             }
-- 
1.7.6.4

[Qemu-devel] [PATCH 3/6] vl: Tighten parsing of -m argument

[Markus Armbruster]

strtosz_suffix() fails unless the size is followed by 0, whitespace or
','.  Useless here, because we need to fail for any junk following the
size, even if it starts with whitespace or ','.  Check manually.
Things like "-m 1024," are now caught.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 vl.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/vl.c b/vl.c
index b9146cf..a50842b 100644
--- a/vl.c
+++ b/vl.c
@@ -2535,9 +2535,10 @@ int main(int argc, char **argv, char **envp)
                 break;
             case QEMU_OPTION_m: {
                 int64_t value;
+                char *end;
 
-                value = strtosz(optarg, NULL);
-                if (value < 0) {
+                value = strtosz(optarg, &end);
+                if (value < 0 || *end) {
                     fprintf(stderr, "qemu: invalid ram size: %s\n", optarg);
                     exit(1);
                 }
-- 
1.7.6.4

[Qemu-devel] [PATCH 4/6] x86/cpuid: Tighten parsing of tsc_freq=FREQ

[Markus Armbruster]

cpu_x86_find_by_name() uses strtosz_suffix_unit(), but screws up the
error checking.  It detects some failures, but not all.  Undetected
failures result in a zero tsc_khz value (error value -1 divided by
1000), which means "no tsc_freq set".

To reproduce, try "-cpu qemu64,tsc_freq=9999999T".
strtosz_suffix_unit() fails, because the value overflows int64_t,

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 target-i386/cpuid.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/target-i386/cpuid.c b/target-i386/cpuid.c
index 21e5896..56c7671 100644
--- a/target-i386/cpuid.c
+++ b/target-i386/cpuid.c
@@ -692,7 +692,7 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model)
 
                 tsc_freq = strtosz_suffix_unit(val, &err,
                                                STRTOSZ_DEFSUFFIX_B, 1000);
-                if (!*val || *err) {
+                if (tsc_freq < 0 || *err) {
                     fprintf(stderr, "bad numerical value %s\n", val);
                     goto error;
                 }
-- 
1.7.6.4

[Qemu-devel] [PATCH 5/6] qemu-img: Tighten parsing of size arguments

[Markus Armbruster]

strtosz_suffix() fails unless the size is followed by 0, whitespace or
','.  Useless here, because we need to fail for any junk following the
size, even if it starts with whitespace or ','.  Check manually.
Things like "qemu-img create xxx 1024," and "qemu-img convert -S '1024
junk'" are now caught.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qemu-img.c |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index 86127f0..8bdae66 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -332,8 +332,9 @@ static int img_create(int argc, char **argv)
     /* Get image size, if specified */
     if (optind < argc) {
         int64_t sval;
-        sval = strtosz_suffix(argv[optind++], NULL, STRTOSZ_DEFSUFFIX_B);
-        if (sval < 0) {
+        char *end;
+        sval = strtosz_suffix(argv[optind++], &end, STRTOSZ_DEFSUFFIX_B);
+        if (sval < 0 || *end) {
             error_report("Invalid image size specified! You may use k, M, G or "
                   "T suffixes for ");
             error_report("kilobytes, megabytes, gigabytes and terabytes.");
@@ -710,8 +711,9 @@ static int img_convert(int argc, char **argv)
         case 'S':
         {
             int64_t sval;
-            sval = strtosz_suffix(optarg, NULL, STRTOSZ_DEFSUFFIX_B);
-            if (sval < 0) {
+            char *end;
+            sval = strtosz_suffix(optarg, &end, STRTOSZ_DEFSUFFIX_B);
+            if (sval < 0 || *end) {
                 error_report("Invalid minimum zero buffer size for sparse output specified");
                 return 1;
             }
-- 
1.7.6.4

[Qemu-devel] [PATCH 6/6] cutils: Make strtosz & friends leave follow set to callers

[Markus Armbruster]

strtosz() & friends require the size to be at the end of the string,
or be followed by whitespace or ','.  I find this surprising, because
the name suggests it works like strtol().

The check simplifies callers that accept exactly that follow set
slightly.  No such callers exist.

The check is redundant for callers that accept a smaller follow set,
and thus need to check themselves anyway.  Right now, this is the case
for all but one caller.  All of them neglected to check, or checked
incorrectly, but the previous few commits fixed them up.

Finally, the check is problematic for callers that accept a larger
follow set.  This is the case in monitor_parse_command().
Fortunately, the problems there are relatively harmless.

monitor_parse_command() uses strtosz() for argument type 'o'.  When
the last argument is of type 'o', a trailing ',' is diagnosed
differently than other trailing junk:

    (qemu) migrate_set_speed 1x
    invalid size
    (qemu) migrate_set_speed 1,
    migrate_set_speed: extraneous characters at the end of line

A related inconsistency exists with non-last arguments.  No such
command exists, but let's use memsave to explore the inconsistency.

The monitor permits, but does not require whitespace between
arguments.  For instance, "memsave (1-1)1024foo" is parsed as command
memsave with three arguments 0, 1024 and "foo".  Yes, this is daft,
but at least it's consistently daft.

If I change memsave's second argument from 'i' to 'o', then "memsave
(1-1)1foo" is rejected, because the size is followed by an 'f'.  But
"memsave (1-1)1," is still accepted, and duly saves to file ",".

We don't have any users of strtosz that profit from the check.  In the
users we have, it appears to encourage sloppy error checking, or gets
in the way.  Drop the bothersome check.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 cutils.c |   70 +++++++++++++++++++++++---------------------------------------
 1 files changed, 26 insertions(+), 44 deletions(-)

diff --git a/cutils.c b/cutils.c
index 55b0596..6db6304 100644
--- a/cutils.c
+++ b/cutils.c
@@ -315,18 +315,34 @@ int fcntl_setfl(int fd, int flag)
 }
 #endif
 
+static int64_t suffix_mul(char suffix, int64_t unit)
+{
+    switch (qemu_toupper(suffix)) {
+    case STRTOSZ_DEFSUFFIX_B:
+        return 1;
+    case STRTOSZ_DEFSUFFIX_KB:
+        return unit;
+    case STRTOSZ_DEFSUFFIX_MB:
+        return unit * unit;
+    case STRTOSZ_DEFSUFFIX_GB:
+        return unit * unit * unit;
+    case STRTOSZ_DEFSUFFIX_TB:
+        return unit * unit * unit * unit;
+    }
+    return -1;
+}
+
 /*
  * Convert string to bytes, allowing either B/b for bytes, K/k for KB,
  * M/m for MB, G/g for GB or T/t for TB. End pointer will be returned
- * in *end, if not NULL. A valid value must be terminated by
- * whitespace, ',' or '\0'. Return -1 on error.
+ * in *end, if not NULL. Return -1 on error.
  */
 int64_t strtosz_suffix_unit(const char *nptr, char **end,
                             const char default_suffix, int64_t unit)
 {
     int64_t retval = -1;
     char *endptr;
-    unsigned char c, d;
+    unsigned char c;
     int mul_required = 0;
     double val, mul, integral, fraction;
 
@@ -339,51 +355,17 @@ int64_t strtosz_suffix_unit(const char *nptr, char **end,
     if (fraction != 0) {
         mul_required = 1;
     }
-    /*
-     * Any whitespace character is fine for terminating the number,
-     * in addition we accept ',' to handle strings where the size is
-     * part of a multi token argument.
-     */
     c = *endptr;
-    d = c;
-    if (qemu_isspace(c) || c == '\0' || c == ',') {
-        c = 0;
-        d = default_suffix;
+    mul = suffix_mul(c, unit);
+    if (mul >= 0) {
+        endptr++;
+    } else {
+        mul = suffix_mul(default_suffix, unit);
+        assert(mul >= 0);
     }
-    switch (qemu_toupper(d)) {
-    case STRTOSZ_DEFSUFFIX_B:
-        mul = 1;
-        if (mul_required) {
-            goto fail;
-        }
-        break;
-    case STRTOSZ_DEFSUFFIX_KB:
-        mul = unit;
-        break;
-    case STRTOSZ_DEFSUFFIX_MB:
-        mul = unit * unit;
-        break;
-    case STRTOSZ_DEFSUFFIX_GB:
-        mul = unit * unit * unit;
-        break;
-    case STRTOSZ_DEFSUFFIX_TB:
-        mul = unit * unit * unit * unit;
-        break;
-    default:
+    if (mul == 1 && mul_required) {
         goto fail;
     }
-    /*
-     * If not terminated by whitespace, ',', or \0, increment endptr
-     * to point to next character, then check that we are terminated
-     * by an appropriate separating character, ie. whitespace, ',', or
-     * \0. If not, we are seeing trailing garbage, thus fail.
-     */
-    if (c != 0) {
-        endptr++;
-        if (!qemu_isspace(*endptr) && *endptr != ',' && *endptr != 0) {
-            goto fail;
-        }
-    }
     if ((val * mul >= INT64_MAX) || val < 0) {
         goto fail;
     }
-- 
1.7.6.4

Re: [Qemu-devel] [PATCH 0/6] Fix strtosz users, clean up its implementation

[Anthony Liguori]

On 11/22/2011 02:46 AM, Markus Armbruster wrote:
> Markus Armbruster (6):
>    cutils: Drop broken support for zero strtosz default_suffix
>    vl: Tighten parsing of -numa's parameter mem
>    vl: Tighten parsing of -m argument
>    x86/cpuid: Tighten parsing of tsc_freq=FREQ
>    qemu-img: Tighten parsing of size arguments
>    cutils: Make strtosz&  friends leave follow set to callers

Applied all.  Thanks.

Regards,

Anthony Liguori

>
>   cutils.c            |   81 +++++++++++++++++----------------------------------
>   qemu-img.c          |   10 ++++--
>   target-i386/cpuid.c |    2 +-
>   vl.c                |    9 +++--
>   4 files changed, 39 insertions(+), 63 deletions(-)
>