[Qemu-devel] [PATCH v3 0/6] Introduce strtosz and make use of it

[Qemu-devel] [PATCH v3 0/6] 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 v3 I tried to apply the changes suggested by Markus, notably I
changed the name from strtobytes() to strtosz(), as well as added
better error handling.

Jes

Jes Sorensen (6):
  Introduce strtosz() library function to convert a string to a byte
    count.
  Support human unit formats in strtosz, eg. 1.0G
  Add more error handling to strtosz()
  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        |   61 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 hmp-commands.hx |    2 +-
 migration.c     |    4 +-
 monitor.c       |   46 ++++++++++++++++++++++++++---------------
 qemu-common.h   |    1 +
 vl.c            |   31 +++++++++-------------------
 6 files changed, 104 insertions(+), 41 deletions(-)

-- 
1.7.2.3

[Qemu-devel] [PATCH 1/6] 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.

v2 renamed from strtobytes() to strtosz() as suggested by Markus.

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

diff --git a/cutils.c b/cutils.c
index 5883737..ee591c5 100644
--- a/cutils.c
+++ b/cutils.c
@@ -283,3 +283,42 @@ int fcntl_setfl(int fd, int flag)
 }
 #endif
 
+/*
+ * Convert string to bytes, allowing either 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)
+{
+    int64_t value;
+    char *endptr;
+
+    value = strtoll(nptr, &endptr, 0);
+    switch (*endptr++) {
+    case 'K':
+    case 'k':
+        value <<= 10;
+        break;
+    case 0:
+    case 'M':
+    case 'm':
+        value <<= 20;
+        break;
+    case 'G':
+    case 'g':
+        value <<= 30;
+        break;
+    case 'T':
+    case 't':
+        value <<= 40;
+        break;
+    default:
+        value = -1;
+    }
+
+    if (end)
+        *end = endptr;
+
+    return value;
+}
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/6] Support human unit formats in strtosz, eg. 1.0G

[Jes.Sorensen]

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

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 cutils.c |   34 ++++++++++++++++++++++++++--------
 1 files changed, 26 insertions(+), 8 deletions(-)

diff --git a/cutils.c b/cutils.c
index ee591c5..0782032 100644
--- a/cutils.c
+++ b/cutils.c
@@ -291,34 +291,52 @@ int fcntl_setfl(int fd, int flag)
  */
 ssize_t strtosz(const char *nptr, char **end)
 {
-    int64_t value;
+    ssize_t retval = -1;
     char *endptr;
+    int mul_required = 0;
+    double val, mul = 1;
+
+    endptr = (char *)nptr + strspn(nptr, " 0123456789");
+    if (*endptr == '.') {
+        mul_required = 1;
+    }
+
+    val = strtod(nptr, &endptr);
+
+    if (val < 0)
+        goto fail;
 
-    value = strtoll(nptr, &endptr, 0);
     switch (*endptr++) {
     case 'K':
     case 'k':
-        value <<= 10;
+        mul = 1 << 10;
         break;
     case 0:
+    case ' ':
+        if (mul_required) {
+            goto fail;
+        }
     case 'M':
     case 'm':
-        value <<= 20;
+        mul = 1ULL << 20;
         break;
     case 'G':
     case 'g':
-        value <<= 30;
+        mul = 1ULL << 30;
         break;
     case 'T':
     case 't':
-        value <<= 40;
+        mul = 1ULL << 40;
         break;
     default:
-        value = -1;
+        goto fail;
     }
 
+    retval = (ssize_t)(val * mul);
+
     if (end)
         *end = endptr;
 
-    return value;
+fail:
+    return retval;
 }
-- 
1.7.2.3

[Qemu-devel] [PATCH 3/6] Add more error handling to strtosz()

[Jes.Sorensen]

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

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 cutils.c |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/cutils.c b/cutils.c
index 0782032..012eb11 100644
--- a/cutils.c
+++ b/cutils.c
@@ -292,6 +292,7 @@ int fcntl_setfl(int fd, int flag)
 ssize_t strtosz(const char *nptr, char **end)
 {
     ssize_t retval = -1;
+    int64_t tmpval;
     char *endptr;
     int mul_required = 0;
     double val, mul = 1;
@@ -301,9 +302,9 @@ ssize_t strtosz(const char *nptr, char **end)
         mul_required = 1;
     }
 
+    errno = 0;
     val = strtod(nptr, &endptr);
-
-    if (val < 0)
+    if (endptr == nptr || errno != 0 || val < 0)
         goto fail;
 
     switch (*endptr++) {
@@ -332,7 +333,10 @@ ssize_t strtosz(const char *nptr, char **end)
         goto fail;
     }
 
-    retval = (ssize_t)(val * mul);
+    tmpval = (val * mul);
+    if (tmpval > ~(size_t)0)
+        goto fail;
+    retval = tmpval;
 
     if (end)
         *end = endptr;
-- 
1.7.2.3

[Qemu-devel] [PATCH 4/6] 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 |   28 ++++++++++++++++++++++++++++
 1 files changed, 28 insertions(+), 0 deletions(-)

diff --git a/monitor.c b/monitor.c
index fbb678d..6dd1926 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,28 @@ 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 +4223,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 5/6] Switch migrate_set_speed() to take an 'o' argument rather than a float.

[Jes.Sorensen]

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

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

diff --git a/hmp-commands.hx b/hmp-commands.hx
index 81999aa..95bdb91 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -754,7 +754,7 @@ ETEXI
 
     {
         .name       = "migrate_set_speed",
-        .args_type  = "value:f",
+        .args_type  = "value:o",
         .params     = "value",
         .help       = "set maximum speed (in bytes) for migrations",
         .user_print = monitor_user_noop,
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 6/6] 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 6dd1926..6678fb5 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
@@ -3726,7 +3722,6 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon,
                 p = end;
             }
             break;
-        case 'f':
         case 'T':
             {
                 double val;
@@ -3742,17 +3737,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;
@@ -4230,7 +4215,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

[Qemu-devel] Re: [PATCH 3/6] Add more error handling to strtosz()

[Paolo Bonzini]

On 10/07/2010 05:01 PM, Jes.Sorensen@redhat.com wrote:
> +    if (tmpval > ~(size_t)0)

Since -1 is an error, this needs to be >=.

Paolo

[Qemu-devel] Re: [PATCH 5/6] Switch migrate_set_speed() to take an 'o' argument rather than a float.

[Paolo Bonzini]

[-- Attachment #1: Type: text/plain, Size: 926 bytes --]

On 10/07/2010 05:01 PM, Jes.Sorensen@redhat.com wrote:
> From: Jes Sorensen<Jes.Sorensen@redhat.com>
>
> Signed-off-by: Jes Sorensen<Jes.Sorensen@redhat.com>
> ---
>   hmp-commands.hx |    2 +-
>   migration.c     |    4 ++--
>   2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 81999aa..95bdb91 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -754,7 +754,7 @@ ETEXI
>
>       {
>           .name       = "migrate_set_speed",
> -        .args_type  = "value:f",
> +        .args_type  = "value:o",
>           .params     = "value",
>           .help       = "set maximum speed (in bytes) for migrations",
                            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Is this still accurate?

Alternatively, you said it's a mess to distinguish the defaults on the 
command line and the monitor, but is it harder than the attached patch?

Paolo

[-- Attachment #2: strtosz.patch --]
[-- Type: text/plain, Size: 2960 bytes --]

diff --git a/cutils.c b/cutils.c
index 012eb11..4a9ce0f 100644
--- a/cutils.c
+++ b/cutils.c
@@ -289,13 +289,13 @@ int fcntl_setfl(int fd, int flag)
  * 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 strtosz(const char *nptr, char **end, int default_shift)
 {
     ssize_t retval = -1;
-    int64_t tmpval;
+    int64_t tmpval, mul;
     char *endptr;
     int mul_required = 0;
-    double val, mul = 1;
+    double val;
 
     endptr = (char *)nptr + strspn(nptr, " 0123456789");
     if (*endptr == '.') {
@@ -308,15 +308,14 @@ ssize_t strtosz(const char *nptr, char **end)
         goto fail;
 
     switch (*endptr++) {
+    case 0:
+    case ' ':
+        mul = (1ULL << default_shift);
+        break;
     case 'K':
     case 'k':
         mul = 1 << 10;
         break;
-    case 0:
-    case ' ':
-        if (mul_required) {
-            goto fail;
-        }
     case 'M':
     case 'm':
         mul = 1ULL << 20;
@@ -333,6 +332,9 @@ ssize_t strtosz(const char *nptr, char **end)
         goto fail;
     }
 
+    if (mul == 1 && mul_required) {
+        goto fail;
+    }
     tmpval = (val * mul);
     if (tmpval > ~(size_t)0)
         goto fail;
diff --git a/monitor.c b/monitor.c
index 6678fb5..0d04b27 100644
--- a/monitor.c
+++ b/monitor.c
@@ -3713,7 +3713,7 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon,
                         break;
                     }
                 }
-                val = strtosz(p, &end);
+                val = strtosz(p, &end, 0);
                 if (val < 0) {
                     monitor_printf(mon, "invalid size\n");
                     goto fail;
diff --git a/qemu-common.h b/qemu-common.h
index 0a062d4..cabd8f8 100644
--- a/qemu-common.h
+++ b/qemu-common.h
@@ -153,7 +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);
+ssize_t strtosz(const char *nptr, char **end, int default_shift);
 
 /* path.c */
 void init_paths(const char *prefix);
diff --git a/vl.c b/vl.c
index 6043fa2..ae4d6f3 100644
--- a/vl.c
+++ b/vl.c
@@ -735,7 +735,7 @@ static void numa_add(const char *optarg)
             node_mem[nodenr] = 0;
         } else {
             ssize_t sval;
-            sval = strtosz(option, NULL);
+            sval = strtosz(option, NULL, 20);
             if (sval < 0) {
                 fprintf(stderr, "qemu: invalid numa mem size: %s\n", optarg);
                 exit(1);
@@ -2162,7 +2162,7 @@ int main(int argc, char **argv, char **envp)
             case QEMU_OPTION_m: {
                 ssize_t value;
 
-                value = strtosz(optarg, NULL);
+                value = strtosz(optarg, NULL, 20);
                 if (value < 0) {
                     fprintf(stderr, "qemu: invalid ram size: %s\n", optarg);
                     exit(1);

[Qemu-devel] Re: [PATCH 5/6] Switch migrate_set_speed() to take an 'o' argument rather than a float.

[Jes Sorensen]

On 10/07/10 18:03, Paolo Bonzini wrote:
> On 10/07/2010 05:01 PM, Jes.Sorensen@redhat.com wrote:
>> diff --git a/hmp-commands.hx b/hmp-commands.hx
>> index 81999aa..95bdb91 100644
>> --- a/hmp-commands.hx
>> +++ b/hmp-commands.hx
>> @@ -754,7 +754,7 @@ ETEXI
>>
>>       {
>>           .name       = "migrate_set_speed",
>> -        .args_type  = "value:f",
>> +        .args_type  = "value:o",
>>           .params     = "value",
>>           .help       = "set maximum speed (in bytes) for migrations",
>                            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> Is this still accurate?
> 
> Alternatively, you said it's a mess to distinguish the defaults on the
> command line and the monitor, but is it harder than the attached patch?

I guess that depends on how you look at it, but I guess I could improve
the error message to make it clearer.

Cheers,
Jes