[Qemu-devel] [PATCH v6 0/4] Introduce strtosz and make use of it

[Qemu-devel] [PATCH v6 0/4] Introduce strtosz and make use of it

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

This patch introduces cutils.c: strtosz() and gets rid of the
multiple custom hacks for parsing byte sizes. In addition it adds
supports for specifying human style sizes such as 1.5G. Last it
eliminates the horrible abuse of a float to store the byte size for
migrate_set_speed in the monitor.

New in v6 I rewrote part of the parsing code as suggested by Markus
and Paolo. The new version relies on strtod to do the actual parsing,
eliminating corner cases not caught by the strspn pass. In addition is
should catch incorrect suffixes that are longer than one character,
and uses isspace() instead of just checking for ' '. Last, a B/b
suffix has been added for 'bytes'.

Jes Sorensen (4):
  Introduce strtosz() library function to convert a string to a byte
    count.
  Add support for 'o' octet (bytes) format as monitor parameter.
  Switch migrate_set_speed() to take an 'o' argument rather than a
    float.
  Remove obsolete 'f' double parameter type

 cutils.c        |   79 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 hmp-commands.hx |    5 ++-
 migration.c     |    4 +-
 monitor.c       |   47 +++++++++++++++++++++------------
 qemu-common.h   |    1 +
 vl.c            |   31 +++++++--------------
 6 files changed, 125 insertions(+), 42 deletions(-)

-- 
1.7.2.3

[Qemu-devel] [PATCH 1/4] Introduce strtosz() library function to convert a string to a byte count.

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

strtosz() returns -1 on error. It now supports human unit formats in
eg. 1.0G, with better error handling.

This version lets strtod() do the actual parsing instead of relying on
strspn, as well as catches NaN input.

The following suffixes are supported:
B/b = bytes
K/k = KB
M/m = MB
G/g = GB
T/t = TB

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 cutils.c      |   79 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 qemu-common.h |    1 +
 vl.c          |   31 +++++++---------------
 3 files changed, 90 insertions(+), 21 deletions(-)

diff --git a/cutils.c b/cutils.c
index 5883737..8e79d92 100644
--- a/cutils.c
+++ b/cutils.c
@@ -23,6 +23,7 @@
  */
 #include "qemu-common.h"
 #include "host-utils.h"
+#include <math.h>
 
 void pstrcpy(char *buf, int buf_size, const char *str)
 {
@@ -283,3 +284,81 @@ int fcntl_setfl(int fd, int flag)
 }
 #endif
 
+/*
+ * 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 end is valid.
+ * Return -1 on error.
+ */
+ssize_t strtosz(const char *nptr, char **end)
+{
+    ssize_t retval = -1;
+    int64_t tmpval;
+    char *endptr, c;
+    int mul_required = 0;
+    double val, mul, integral, fraction;
+
+    errno = 0;
+    val = strtod(nptr, &endptr);
+    if (isnan(val) || endptr == nptr || errno != 0 || val < 0 ||
+        val == HUGE_VAL) {
+        goto fail;
+    }
+    integral = modf(val, &fraction);
+    if (integral != 0) {
+        mul_required = 1;
+    }
+    /*
+     * Any whitespace character is fine for terminating the number,
+     * otherwise check that the suffix is just one character.
+     */
+    c = *endptr++;
+    if (isspace(c) || c == '\0') {
+        c = 0;
+    } else if (!isspace(*endptr) && *endptr != 0) {
+        goto fail;
+    }
+    switch (c) {
+    case 'B':
+    case 'b':
+        mul = 1;
+        if (mul_required) {
+            goto fail;
+        }
+        break;
+    case 'K':
+    case 'k':
+        mul = 1 << 10;
+        break;
+    case 0:
+        if (mul_required) {
+            goto fail;
+        }
+    case 'M':
+    case 'm':
+        mul = 1ULL << 20;
+        break;
+    case 'G':
+    case 'g':
+        mul = 1ULL << 30;
+        break;
+    case 'T':
+    case 't':
+        mul = 1ULL << 40;
+        break;
+    default:
+        goto fail;
+    }
+    tmpval = (val * mul);
+    if (tmpval >= ~(size_t)0) {
+        goto fail;
+    }
+    retval = tmpval;
+
+    if (end) {
+        *end = endptr;
+    }
+
+fail:
+    return retval;
+}
diff --git a/qemu-common.h b/qemu-common.h
index 81aafa0..0a062d4 100644
--- a/qemu-common.h
+++ b/qemu-common.h
@@ -153,6 +153,7 @@ time_t mktimegm(struct tm *tm);
 int qemu_fls(int i);
 int qemu_fdatasync(int fd);
 int fcntl_setfl(int fd, int flag);
+ssize_t strtosz(const char *nptr, char **end);
 
 /* path.c */
 void init_paths(const char *prefix);
diff --git a/vl.c b/vl.c
index df414ef..6043fa2 100644
--- a/vl.c
+++ b/vl.c
@@ -734,16 +734,13 @@ static void numa_add(const char *optarg)
         if (get_param_value(option, 128, "mem", optarg) == 0) {
             node_mem[nodenr] = 0;
         } else {
-            value = strtoull(option, &endptr, 0);
-            switch (*endptr) {
-            case 0: case 'M': case 'm':
-                value <<= 20;
-                break;
-            case 'G': case 'g':
-                value <<= 30;
-                break;
+            ssize_t sval;
+            sval = strtosz(option, NULL);
+            if (sval < 0) {
+                fprintf(stderr, "qemu: invalid numa mem size: %s\n", optarg);
+                exit(1);
             }
-            node_mem[nodenr] = value;
+            node_mem[nodenr] = sval;
         }
         if (get_param_value(option, 128, "cpus", optarg) == 0) {
             node_cpumask[nodenr] = 0;
@@ -2163,18 +2160,10 @@ int main(int argc, char **argv, char **envp)
                 exit(0);
                 break;
             case QEMU_OPTION_m: {
-                uint64_t value;
-                char *ptr;
+                ssize_t value;
 
-                value = strtoul(optarg, &ptr, 10);
-                switch (*ptr) {
-                case 0: case 'M': case 'm':
-                    value <<= 20;
-                    break;
-                case 'G': case 'g':
-                    value <<= 30;
-                    break;
-                default:
+                value = strtosz(optarg, NULL);
+                if (value < 0) {
                     fprintf(stderr, "qemu: invalid ram size: %s\n", optarg);
                     exit(1);
                 }
-- 
1.7.2.3

[Qemu-devel] [PATCH 2/4] Add support for 'o' octet (bytes) format as monitor parameter.

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

Octet format relies on strtosz which supports K/k, M/m, G/g, T/t
suffixes and unit support for humans, like 1.3G

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 monitor.c |   29 +++++++++++++++++++++++++++++
 1 files changed, 29 insertions(+), 0 deletions(-)

diff --git a/monitor.c b/monitor.c
index fbb678d..b0ea5a3 100644
--- a/monitor.c
+++ b/monitor.c
@@ -78,6 +78,11 @@
  * 'l'          target long (32 or 64 bit)
  * 'M'          just like 'l', except in user mode the value is
  *              multiplied by 2^20 (think Mebibyte)
+ * 'o'          octets (aka bytes)
+ *              user mode accepts an optional T, t, G, g, M, m, K, k
+ *              suffix, which multiplies the value by 2^40 for
+ *              suffixes T and t, 2^30 for suffixes G and g, 2^20 for
+ *              M and m, 2^10 for K and k
  * 'f'          double
  *              user mode accepts an optional G, g, M, m, K, k suffix,
  *              which multiplies the value by 2^30 for suffixes G and
@@ -3699,6 +3704,29 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon,
                 qdict_put(qdict, key, qint_from_int(val));
             }
             break;
+        case 'o':
+            {
+                ssize_t val;
+                char *end;
+
+                while (qemu_isspace(*p)) {
+                    p++;
+                }
+                if (*typestr == '?') {
+                    typestr++;
+                    if (*p == '\0') {
+                        break;
+                    }
+                }
+                val = strtosz(p, &end);
+                if (val < 0) {
+                    monitor_printf(mon, "invalid size\n");
+                    goto fail;
+                }
+                qdict_put(qdict, key, qint_from_int(val));
+                p = end;
+            }
+            break;
         case 'f':
         case 'T':
             {
@@ -4196,6 +4224,7 @@ static int check_client_args_type(const QDict *client_args,
         case 'i':
         case 'l':
         case 'M':
+        case 'o':
             if (qobject_type(client_arg) != QTYPE_QINT) {
                 qerror_report(QERR_INVALID_PARAMETER_TYPE, client_arg_name,
                               "int");
-- 
1.7.2.3

[Qemu-devel] [PATCH 3/4] Switch migrate_set_speed() to take an 'o' argument rather than a float.

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

Clarify default value of MB in migration speed argument in monitor, if
no suffix is specified. This differ from previous default of bytes,
but is consistent with the rest of the places where we accept a size
argument.

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 hmp-commands.hx |    5 +++--
 migration.c     |    4 ++--
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index 81999aa..e5585ba 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -754,9 +754,10 @@ ETEXI
 
     {
         .name       = "migrate_set_speed",
-        .args_type  = "value:f",
+        .args_type  = "value:o",
         .params     = "value",
-        .help       = "set maximum speed (in bytes) for migrations",
+        .help       = "set maximum speed (in bytes) for migrations. "
+	"Defaults to MB if no size suffix is specified, ie. B/K/M/G/T",
         .user_print = monitor_user_noop,
         .mhandler.cmd_new = do_migrate_set_speed,
     },
diff --git a/migration.c b/migration.c
index 468d517..9ee8b17 100644
--- a/migration.c
+++ b/migration.c
@@ -132,10 +132,10 @@ int do_migrate_cancel(Monitor *mon, const QDict *qdict, QObject **ret_data)
 
 int do_migrate_set_speed(Monitor *mon, const QDict *qdict, QObject **ret_data)
 {
-    double d;
+    int64_t d;
     FdMigrationState *s;
 
-    d = qdict_get_double(qdict, "value");
+    d = qdict_get_int(qdict, "value");
     d = MAX(0, MIN(UINT32_MAX, d));
     max_throttle = d;
 
-- 
1.7.2.3

[Qemu-devel] [PATCH 4/4] Remove obsolete 'f' double parameter type

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

'f' double is no longer used, and we should be using floating point
variables to store byte sizes. Remove it.

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 monitor.c |   18 +-----------------
 1 files changed, 1 insertions(+), 17 deletions(-)

diff --git a/monitor.c b/monitor.c
index b0ea5a3..078a66e 100644
--- a/monitor.c
+++ b/monitor.c
@@ -83,10 +83,6 @@
  *              suffix, which multiplies the value by 2^40 for
  *              suffixes T and t, 2^30 for suffixes G and g, 2^20 for
  *              M and m, 2^10 for K and k
- * 'f'          double
- *              user mode accepts an optional G, g, M, m, K, k suffix,
- *              which multiplies the value by 2^30 for suffixes G and
- *              g, 2^20 for M and m, 2^10 for K and k
  * 'T'          double
  *              user mode accepts an optional ms, us, ns suffix,
  *              which divides the value by 1e3, 1e6, 1e9, respectively
@@ -3727,7 +3723,6 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon,
                 p = end;
             }
             break;
-        case 'f':
         case 'T':
             {
                 double val;
@@ -3743,17 +3738,7 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon,
                 if (get_double(mon, &val, &p) < 0) {
                     goto fail;
                 }
-                if (c == 'f' && *p) {
-                    switch (*p) {
-                    case 'K': case 'k':
-                        val *= 1 << 10; p++; break;
-                    case 'M': case 'm':
-                        val *= 1 << 20; p++; break;
-                    case 'G': case 'g':
-                        val *= 1 << 30; p++; break;
-                    }
-                }
-                if (c == 'T' && p[0] && p[1] == 's') {
+                if (p[0] && p[1] == 's') {
                     switch (*p) {
                     case 'm':
                         val /= 1e3; p += 2; break;
@@ -4231,7 +4216,6 @@ static int check_client_args_type(const QDict *client_args,
                 return -1; 
             }
             break;
-        case 'f':
         case 'T':
             if (qobject_type(client_arg) != QTYPE_QINT &&
                 qobject_type(client_arg) != QTYPE_QFLOAT) {
-- 
1.7.2.3

Re: [Qemu-devel] [PATCH 1/4] Introduce strtosz() library function to convert a string to a byte count.

[Markus Armbruster]

Still not entirely happy, but maybe we can commit it as is, and fix it
up later.

Jes.Sorensen@redhat.com writes:

> From: Jes Sorensen <Jes.Sorensen@redhat.com>
>
> strtosz() returns -1 on error. It now supports human unit formats in
> eg. 1.0G, with better error handling.
>
> This version lets strtod() do the actual parsing instead of relying on
> strspn, as well as catches NaN input.

This is information on what changed since v5, and as such it doesn't
belong into the commit message.

> The following suffixes are supported:
> B/b = bytes
> K/k = KB
> M/m = MB
> G/g = GB
> T/t = TB
>
> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>

Would be nice if commit message documented that this affects -numa and
-m.  In particular that they now accept more suffixes than before.

> ---
>  cutils.c      |   79 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  qemu-common.h |    1 +
>  vl.c          |   31 +++++++---------------
>  3 files changed, 90 insertions(+), 21 deletions(-)
>
> diff --git a/cutils.c b/cutils.c
> index 5883737..8e79d92 100644
> --- a/cutils.c
> +++ b/cutils.c
> @@ -23,6 +23,7 @@
>   */
>  #include "qemu-common.h"
>  #include "host-utils.h"
> +#include <math.h>
>  
>  void pstrcpy(char *buf, int buf_size, const char *str)
>  {
> @@ -283,3 +284,81 @@ int fcntl_setfl(int fd, int flag)
>  }
>  #endif
>  
> +/*
> + * 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 end is valid.

Nitpick: There are plenty of invalid pointers we'll happily attempt to
use.  "unless end is null" would be more precise.

> + * Return -1 on error.
> + */
> +ssize_t strtosz(const char *nptr, char **end)
> +{
> +    ssize_t retval = -1;
> +    int64_t tmpval;
> +    char *endptr, c;
> +    int mul_required = 0;
> +    double val, mul, integral, fraction;
> +
> +    errno = 0;
> +    val = strtod(nptr, &endptr);
> +    if (isnan(val) || endptr == nptr || errno != 0 || val < 0 ||
> +        val == HUGE_VAL) {

ISO C permits implementations supporting infinities to make HUGE_VAL
*not* +inf.  So this may not catch +inf.  val >= HUGE_VAL would.

But since we have to catch val * mul out of range further down anyway,
the check for HUGE_VAL may be redundant here.

> +        goto fail;
> +    }
> +    integral = modf(val, &fraction);
> +    if (integral != 0) {
> +        mul_required = 1;
> +    }
> +    /*
> +     * Any whitespace character is fine for terminating the number,
> +     * otherwise check that the suffix is just one character.
> +     */
> +    c = *endptr++;
> +    if (isspace(c) || c == '\0') {
> +        c = 0;
> +    } else if (!isspace(*endptr) && *endptr != 0) {
> +        goto fail;
> +    }

I'm not happy with this check.

If the caller needs a complete string consumed, then this check is
insufficient, because it doesn't catch trailing garbage as long as it
starts with whitespace.  The caller still needs to check !*endptr.

If the caller needs to continue parsing after the value, and expects
anything but whitespace there, it has to copy the value first.  Only
easy if the value is followed by some delimiter that can't occur in the
value.  Example: parse a size value from something of them form
name=value,name=value...  Need to copy up to the next comma or end of
string.

The check complicates the second case without really helping the first
case.

Nevertheless, it's good enough for the uses in this patch series, so I'm
not insisting on getting this changed now.

> +    switch (c) {
> +    case 'B':
> +    case 'b':
> +        mul = 1;
> +        if (mul_required) {
> +            goto fail;
> +        }
> +        break;
> +    case 'K':
> +    case 'k':
> +        mul = 1 << 10;
> +        break;
> +    case 0:
> +        if (mul_required) {
> +            goto fail;
> +        }
> +    case 'M':
> +    case 'm':
> +        mul = 1ULL << 20;
> +        break;
> +    case 'G':
> +    case 'g':
> +        mul = 1ULL << 30;
> +        break;
> +    case 'T':
> +    case 't':
> +        mul = 1ULL << 40;
> +        break;
> +    default:
> +        goto fail;
> +    }
> +    tmpval = (val * mul);
> +    if (tmpval >= ~(size_t)0) {
> +        goto fail;

val * mul may exceed the range of int64_t tmpval, and then the
assignment has undefined behavior.  Obvious way to avoid that:

    if (val * mul >= ~(size_t)0) {
        goto fail;
    }
    retval = val * mul;

> +    }
> +    retval = tmpval;
> +
> +    if (end) {
> +        *end = endptr;
> +    }
> +
> +fail:
> +    return retval;
> +}
> diff --git a/qemu-common.h b/qemu-common.h
> index 81aafa0..0a062d4 100644
> --- a/qemu-common.h
> +++ b/qemu-common.h
> @@ -153,6 +153,7 @@ time_t mktimegm(struct tm *tm);
>  int qemu_fls(int i);
>  int qemu_fdatasync(int fd);
>  int fcntl_setfl(int fd, int flag);
> +ssize_t strtosz(const char *nptr, char **end);
>  
>  /* path.c */
>  void init_paths(const char *prefix);
> diff --git a/vl.c b/vl.c
> index df414ef..6043fa2 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -734,16 +734,13 @@ static void numa_add(const char *optarg)
>          if (get_param_value(option, 128, "mem", optarg) == 0) {
>              node_mem[nodenr] = 0;
>          } else {
> -            value = strtoull(option, &endptr, 0);
> -            switch (*endptr) {
> -            case 0: case 'M': case 'm':
> -                value <<= 20;
> -                break;
> -            case 'G': case 'g':
> -                value <<= 30;
> -                break;
> +            ssize_t sval;
> +            sval = strtosz(option, NULL);
> +            if (sval < 0) {
> +                fprintf(stderr, "qemu: invalid numa mem size: %s\n", optarg);
> +                exit(1);
>              }
> -            node_mem[nodenr] = value;
> +            node_mem[nodenr] = sval;
>          }
>          if (get_param_value(option, 128, "cpus", optarg) == 0) {
>              node_cpumask[nodenr] = 0;
> @@ -2163,18 +2160,10 @@ int main(int argc, char **argv, char **envp)
>                  exit(0);
>                  break;
>              case QEMU_OPTION_m: {
> -                uint64_t value;
> -                char *ptr;
> +                ssize_t value;
>  
> -                value = strtoul(optarg, &ptr, 10);
> -                switch (*ptr) {
> -                case 0: case 'M': case 'm':
> -                    value <<= 20;
> -                    break;
> -                case 'G': case 'g':
> -                    value <<= 30;
> -                    break;
> -                default:
> +                value = strtosz(optarg, NULL);
> +                if (value < 0) {
>                      fprintf(stderr, "qemu: invalid ram size: %s\n", optarg);
>                      exit(1);
>                  }

Re: [Qemu-devel] [PATCH 1/4] Introduce strtosz() library function to convert a string to a byte count.

[Jes Sorensen]

On 10/12/10 17:52, Markus Armbruster wrote:
> Still not entirely happy, but maybe we can commit it as is, and fix it
> up later.

No worries, I think this is the most serious review I have ever received
for any piece of code, but you're finding valid points so it's good. If
all of QEMU had been reviewed like this we would be in really good shape :)

>> The following suffixes are supported:
>> B/b = bytes
>> K/k = KB
>> M/m = MB
>> G/g = GB
>> T/t = TB
>>
>> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
> 
> Would be nice if commit message documented that this affects -numa and
> -m.  In particular that they now accept more suffixes than before.

Will address this in the commit message.

>> +/*
>> + * 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 end is valid.
> 
> Nitpick: There are plenty of invalid pointers we'll happily attempt to
> use.  "unless end is null" would be more precise.

Fixed

>> +    errno = 0;
>> +    val = strtod(nptr, &endptr);
>> +    if (isnan(val) || endptr == nptr || errno != 0 || val < 0 ||
>> +        val == HUGE_VAL) {
> 
> ISO C permits implementations supporting infinities to make HUGE_VAL
> *not* +inf.  So this may not catch +inf.  val >= HUGE_VAL would.
> 
> But since we have to catch val * mul out of range further down anyway,
> the check for HUGE_VAL may be redundant here.

Valid point, fixed in the upcoming version.

>> +    c = *endptr++;
>> +    if (isspace(c) || c == '\0') {
>> +        c = 0;
>> +    } else if (!isspace(*endptr) && *endptr != 0) {
>> +        goto fail;
>> +    }
> 
> I'm not happy with this check.
> 
> If the caller needs a complete string consumed, then this check is
> insufficient, because it doesn't catch trailing garbage as long as it
> starts with whitespace.  The caller still needs to check !*endptr.
> 
> If the caller needs to continue parsing after the value, and expects
> anything but whitespace there, it has to copy the value first.  Only
> easy if the value is followed by some delimiter that can't occur in the
> value.  Example: parse a size value from something of them form
> name=value,name=value...  Need to copy up to the next comma or end of
> string.
> 
> The check complicates the second case without really helping the first
> case.
> 
> Nevertheless, it's good enough for the uses in this patch series, so I'm
> not insisting on getting this changed now.

I hadn't thought of case #2, but I think that is pretty easy to handle
by accepting ',' as a separator as well. It's worth keeping in kind that
the old code didn't do anything with trailing garbage either, it was
silently ignored.

For case #1 then I think it's ok to just accept trailing garbage, the
old code would simply use strtoull and leave it at that.

>> +    tmpval = (val * mul);
>> +    if (tmpval >= ~(size_t)0) {
>> +        goto fail;
> 
> val * mul may exceed the range of int64_t tmpval, and then the
> assignment has undefined behavior.  Obvious way to avoid that:
> 
>     if (val * mul >= ~(size_t)0) {
>         goto fail;
>     }
>     retval = val * mul;

Good point, fixed.

Updated version coming up shortly.

Jes

Re: [Qemu-devel] [PATCH 1/4] Introduce strtosz() library function to convert a string to a byte count.

[Markus Armbruster]

Jes Sorensen <Jes.Sorensen@redhat.com> writes:

> On 10/12/10 17:52, Markus Armbruster wrote:
[...]
>>> +    c = *endptr++;
>>> +    if (isspace(c) || c == '\0') {
>>> +        c = 0;
>>> +    } else if (!isspace(*endptr) && *endptr != 0) {
>>> +        goto fail;
>>> +    }
>> 
>> I'm not happy with this check.
>> 
>> If the caller needs a complete string consumed, then this check is
>> insufficient, because it doesn't catch trailing garbage as long as it
>> starts with whitespace.  The caller still needs to check !*endptr.
>> 
>> If the caller needs to continue parsing after the value, and expects
>> anything but whitespace there, it has to copy the value first.  Only
>> easy if the value is followed by some delimiter that can't occur in the
>> value.  Example: parse a size value from something of them form
>> name=value,name=value...  Need to copy up to the next comma or end of
>> string.
>> 
>> The check complicates the second case without really helping the first
>> case.
>> 
>> Nevertheless, it's good enough for the uses in this patch series, so I'm
>> not insisting on getting this changed now.
>
> I hadn't thought of case #2, but I think that is pretty easy to handle
> by accepting ',' as a separator as well.

Then callers that don't want ',' have to check the suffix.

And when a caller comes around that wants '"', it has to copy the value
string, or hack strtosz() and figure out which of the existing callers
now have to check the suffix.

strtosz() shouldn't second guess what characters can legitimately follow
(the follow set in parsing parlance).  That's only known up the call
chain.

I still think there are two clean interfaces:

* Consume the longest prefix that makes sense, then stop.  Return where
  we stopped.  Leave parsing / rejecting the suffix to the caller.
  That's how strtol() & friends work.

* Consume a complete string, fail if there's trailing garbage.  This is
  easier to use when you want to parse complete strings.  It's awkward
  when you don't.

>                                          It's worth keeping in kind that
> the old code didn't do anything with trailing garbage either, it was
> silently ignored.
>
> For case #1 then I think it's ok to just accept trailing garbage, the
> old code would simply use strtoull and leave it at that.

As long as your patch doesn't make things worse, it can be committed.
When I come across another use of strtosz() where it doesn't work
nicely, I can fix it up.

[...]