[Qemu-devel] [PATCH v2 0/5] Introduce strtobytes and make use of it

[Qemu-devel] [PATCH v2 0/5] Introduce strtobytes and make use of it

[Jes.Sorensen]

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

This patch introduces cutils.c: strtobytes() and gets rid of the
multiple custom hacks for parsing byte sizes. In addition it add
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 v2 I changed it to use strtod() instead, losely based on
Paolo's suggestion, plus fixed a typo as pointed out by Chris Krumme.
Only patches 2 and 3 have changed.
Jes

Jes Sorensen (5):
  Introduce strtobytes() library function to convert string to byte
    count.
  Support human unit formats in strtobytes, eg. 1.0G
  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        |   57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 migration.c     |    4 +-
 monitor.c       |   44 +++++++++++++++++++++++++++---------------
 qemu-common.h   |    1 +
 qemu-monitor.hx |    2 +-
 vl.c            |   26 ++++++------------------
 6 files changed, 96 insertions(+), 38 deletions(-)

-- 
1.7.2.3

[Qemu-devel] [PATCH 1/5] Introduce strtobytes() library function to convert string to byte count.

[Jes.Sorensen]

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

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

diff --git a/cutils.c b/cutils.c
index 036ae3c..dabbed4 100644
--- a/cutils.c
+++ b/cutils.c
@@ -251,3 +251,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 0 on error.
+ */
+uint64_t strtobytes(const char *nptr, char **end)
+{
+    uint64_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 = 0;
+    }
+
+    if (end)
+        *end = endptr;
+
+    return value;
+}
diff --git a/qemu-common.h b/qemu-common.h
index dfd3dc0..01d7dcb 100644
--- a/qemu-common.h
+++ b/qemu-common.h
@@ -137,6 +137,7 @@ time_t mktimegm(struct tm *tm);
 int qemu_fls(int i);
 int qemu_fdatasync(int fd);
 int fcntl_setfl(int fd, int flag);
+uint64_t strtobytes(const char *nptr, char **end);
 
 /* path.c */
 void init_paths(const char *prefix);
diff --git a/vl.c b/vl.c
index 3f45aa9..0cdd94a 100644
--- a/vl.c
+++ b/vl.c
@@ -734,14 +734,10 @@ 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;
+            value = strtobytes(option, NULL);
+            if (!value) {
+                fprintf(stderr, "qemu: invalid numa mem size: %s\n", optarg);
+                exit(1);
             }
             node_mem[nodenr] = value;
         }
@@ -2166,17 +2162,9 @@ int main(int argc, char **argv, char **envp)
                 break;
             case QEMU_OPTION_m: {
                 uint64_t value;
-                char *ptr;
 
-                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 = strtobytes(optarg, NULL);
+                if (!value) {
                     fprintf(stderr, "qemu: invalid ram size: %s\n", optarg);
                     exit(1);
                 }
-- 
1.7.2.3

[Qemu-devel] [PATCH 2/5] Support human unit formats in strtobytes, 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 dabbed4..67767a9 100644
--- a/cutils.c
+++ b/cutils.c
@@ -259,34 +259,52 @@ int fcntl_setfl(int fd, int flag)
  */
 uint64_t strtobytes(const char *nptr, char **end)
 {
-    uint64_t value;
+    uint64_t retval = 0;
     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 = 0;
+        goto fail;
     }
 
+    retval = (uint64_t)(val * mul);
+
     if (end)
         *end = endptr;
 
-    return value;
+fail:
+    return retval;
 }
-- 
1.7.2.3

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

[Jes.Sorensen]

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

Octet format relies on strtobytes 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 |   27 +++++++++++++++++++++++++++
 1 files changed, 27 insertions(+), 0 deletions(-)

diff --git a/monitor.c b/monitor.c
index e602480..3630061 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
@@ -3594,6 +3599,28 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon,
                 qdict_put(qdict, key, qint_from_int(val));
             }
             break;
+        case 'o':
+            {
+                int64_t val;
+                char *end;
+
+                while (qemu_isspace(*p))
+                    p++;
+                if (*typestr == '?') {
+                    typestr++;
+                    if (*p == '\0') {
+                        break;
+                    }
+                }
+                val = strtobytes(p, &end);
+                if (!val) {
+                    monitor_printf(mon, "invalid size\n");
+                    goto fail;
+                }
+                qdict_put(qdict, key, qint_from_int(val));
+                p = end;
+            }
+            break;
         case 'f':
         case 'T':
             {
-- 
1.7.2.3

[Qemu-devel] [PATCH 4/5] 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>
---
 migration.c     |    4 ++--
 qemu-monitor.hx |    2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

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;
 
diff --git a/qemu-monitor.hx b/qemu-monitor.hx
index 49bcd8d..7f58fb2 100644
--- a/qemu-monitor.hx
+++ b/qemu-monitor.hx
@@ -1093,7 +1093,7 @@ EQMP
 
     {
         .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,
-- 
1.7.2.3

[Qemu-devel] [PATCH 5/5] 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 |   17 +----------------
 1 files changed, 1 insertions(+), 16 deletions(-)

diff --git a/monitor.c b/monitor.c
index 3630061..ffeb4ee 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
@@ -3621,7 +3617,6 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon,
                 p = end;
             }
             break;
-        case 'f':
         case 'T':
             {
                 double val;
@@ -3637,17 +3632,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;
-- 
1.7.2.3

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

[Markus Armbruster]

Jes.Sorensen@redhat.com writes:

> From: Jes Sorensen <Jes.Sorensen@redhat.com>
>
> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
> ---
>  cutils.c      |   39 +++++++++++++++++++++++++++++++++++++++
>  qemu-common.h |    1 +
>  vl.c          |   26 +++++++-------------------
>  3 files changed, 47 insertions(+), 19 deletions(-)
>
> diff --git a/cutils.c b/cutils.c
> index 036ae3c..dabbed4 100644
> --- a/cutils.c
> +++ b/cutils.c
> @@ -251,3 +251,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 0 on error.

Funny error value.  Perhaps a more conventional choice would be
(uint64_t)-1.

> + */
> +uint64_t strtobytes(const char *nptr, char **end)

I don't like the name.  It suggests the function parses a number of
bytes from the string.

size_t strtoz() would make sense (think printf/scanf conversion
specifier "%zd").  But only if you use size_t.  If not, then perhaps
strtosize() or strtocount().

> +{
> +    uint64_t value;
> +    char *endptr;
> +
> +    value = strtoll(nptr, &endptr, 0);

Unsigned variable, signed parsing function.  What about making the two
consistent?  unsigned long long value = stroull(...)

Unless you change the return type as well, you then have to worry about
uint64_t having a different width than unsigned long long.  Cleanest
solution would be parsing into uintmax_t with strtoumax(), then checked
conversion to uint64_t.

Next, this seems to accept blank input.  Intentional?  Works out because
stroull() returns zero then, which happens to be your error value.

Furthermore, you don't detect overflow here.  If you want to do that,
set errno = 0 before, and check it afterwards.

Here's the idiomatic way to use strtol() & friends correctly:

    errno = 0;
    l = strtol(str, end, 10);
    if (*end == str || errno != 0)
        FAIL();

> +    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 = 0;
> +    }
> +
> +    if (end)
> +        *end = endptr;
> +
> +    return value;
> +}
> diff --git a/qemu-common.h b/qemu-common.h
> index dfd3dc0..01d7dcb 100644
> --- a/qemu-common.h
> +++ b/qemu-common.h
> @@ -137,6 +137,7 @@ time_t mktimegm(struct tm *tm);
>  int qemu_fls(int i);
>  int qemu_fdatasync(int fd);
>  int fcntl_setfl(int fd, int flag);
> +uint64_t strtobytes(const char *nptr, char **end);
>  
>  /* path.c */
>  void init_paths(const char *prefix);
> diff --git a/vl.c b/vl.c
> index 3f45aa9..0cdd94a 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -734,14 +734,10 @@ 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;
> +            value = strtobytes(option, NULL);
> +            if (!value) {
> +                fprintf(stderr, "qemu: invalid numa mem size: %s\n", optarg);
> +                exit(1);
>              }
>              node_mem[nodenr] = value;
>          }

You add proper error handling where there was none before.  That's good,
but the commit message doesn't mention it.  Separate patch?

> @@ -2166,17 +2162,9 @@ int main(int argc, char **argv, char **envp)
>                  break;
>              case QEMU_OPTION_m: {
>                  uint64_t value;
> -                char *ptr;
>  
> -                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 = strtobytes(optarg, NULL);
> +                if (!value) {
>                      fprintf(stderr, "qemu: invalid ram size: %s\n", optarg);
>                      exit(1);
>                  }

Re: [Qemu-devel] [PATCH 2/5] Support human unit formats in strtobytes, eg. 1.0G

[Markus Armbruster]

Jes.Sorensen@redhat.com writes:

> 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 dabbed4..67767a9 100644
> --- a/cutils.c
> +++ b/cutils.c
> @@ -259,34 +259,52 @@ int fcntl_setfl(int fd, int flag)
>   */
>  uint64_t strtobytes(const char *nptr, char **end)
>  {
> -    uint64_t value;
> +    uint64_t retval = 0;
>      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;

I pointed out several problems with the use of strtoull() in PATCH 1/5,
and now you go ahead and switch over to strtod() in PATCH 2/5.  Gee,
thanks!  For penance, I want you to peruse strtod(3) and figure out its
correct usage ;)

>  
> -    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;
> +        }

I wouldn't have bothered catching this case.  Instead, I'd have checked
that the final conversion to uint64_t is exact.  But since you coded it
already, feel free to keep it.

>      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 = 0;
> +        goto fail;
>      }
>  
> +    retval = (uint64_t)(val * mul);
> +

What if the conversion overflows uint64_t?  Shouldn't we catch that?

>      if (end)
>          *end = endptr;
>  
> -    return value;
> +fail:
> +    return retval;
>  }

Re: [Qemu-devel] [PATCH 3/5] Add support for 'o' octet (bytes) format as monitor parameter.

[Markus Armbruster]

Jes.Sorensen@redhat.com writes:

> From: Jes Sorensen <Jes.Sorensen@redhat.com>
>
> Octet format relies on strtobytes 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 |   27 +++++++++++++++++++++++++++
>  1 files changed, 27 insertions(+), 0 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index e602480..3630061 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
> @@ -3594,6 +3599,28 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon,
>                  qdict_put(qdict, key, qint_from_int(val));
>              }
>              break;
> +        case 'o':
> +            {
> +                int64_t val;
> +                char *end;
> +
> +                while (qemu_isspace(*p))
> +                    p++;
> +                if (*typestr == '?') {
> +                    typestr++;
> +                    if (*p == '\0') {
> +                        break;
> +                    }
> +                }
> +                val = strtobytes(p, &end);
> +                if (!val) {
> +                    monitor_printf(mon, "invalid size\n");
> +                    goto fail;
> +                }
> +                qdict_put(qdict, key, qint_from_int(val));
> +                p = end;
> +            }
> +            break;
>          case 'f':
>          case 'T':
>              {

This is incomplete, you need to update check_client_args_type() as
well.  Testing with QMP should have made that obvious.

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

[Markus Armbruster]

Jes.Sorensen@redhat.com writes:

> From: Jes Sorensen <Jes.Sorensen@redhat.com>
>
> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
> ---
>  migration.c     |    4 ++--
>  qemu-monitor.hx |    2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)
>
> 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;

This isn't backwards bug-compatible.

Before, a client could send any number.  Any fractional part was
ignored.

Now, the number must be an integer.  Other numbers are rejected.

I don't care myself, but others have argued most forcefully for keeping
QMP fully backward compatible from 0.13 on, so they might object.

> diff --git a/qemu-monitor.hx b/qemu-monitor.hx
> index 49bcd8d..7f58fb2 100644
> --- a/qemu-monitor.hx
> +++ b/qemu-monitor.hx
> @@ -1093,7 +1093,7 @@ EQMP
>  
>      {
>          .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,

Doesn't this change the interpretation of "42" from 42 to (42 << 20)?

Re: [Qemu-devel] [PATCH 5/5] Remove obsolete 'f' double parameter type

[Markus Armbruster]

Jes.Sorensen@redhat.com writes:

> 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 |   17 +----------------
>  1 files changed, 1 insertions(+), 16 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index 3630061..ffeb4ee 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
> @@ -3621,7 +3617,6 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon,
>                  p = end;
>              }
>              break;
> -        case 'f':
>          case 'T':
>              {
>                  double val;
> @@ -3637,17 +3632,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;

You missed check_client_args_type().

Yes, the monitor code sucks rocks.

Re: [Qemu-devel] [PATCH 3/5] Add support for 'o' octet (bytes) format as monitor parameter.

[Luiz Capitulino]

On Tue, 28 Sep 2010 12:06:01 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Jes.Sorensen@redhat.com writes:
> 
> > From: Jes Sorensen <Jes.Sorensen@redhat.com>
> >
> > Octet format relies on strtobytes 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 |   27 +++++++++++++++++++++++++++
> >  1 files changed, 27 insertions(+), 0 deletions(-)
> >
> > diff --git a/monitor.c b/monitor.c
> > index e602480..3630061 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
> > @@ -3594,6 +3599,28 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon,
> >                  qdict_put(qdict, key, qint_from_int(val));
> >              }
> >              break;
> > +        case 'o':
> > +            {
> > +                int64_t val;
> > +                char *end;
> > +
> > +                while (qemu_isspace(*p))
> > +                    p++;
> > +                if (*typestr == '?') {
> > +                    typestr++;
> > +                    if (*p == '\0') {
> > +                        break;
> > +                    }
> > +                }
> > +                val = strtobytes(p, &end);
> > +                if (!val) {
> > +                    monitor_printf(mon, "invalid size\n");
> > +                    goto fail;
> > +                }
> > +                qdict_put(qdict, key, qint_from_int(val));
> > +                p = end;
> > +            }
> > +            break;
> >          case 'f':
> >          case 'T':
> >              {
> 
> This is incomplete, you need to update check_client_args_type() as
> well.  Testing with QMP should have made that obvious.

Right and we have the same issue with 'f' as you pointed out.

However, the real problem here is that I've duplicated this stuff
in QMP.

I see two solutions:

 1. Move all this stuff to common code (say monitor_args.c). This is
    easy and quick

 2. Move QMP away from args_type. This is probably the right thing to
    do, but then I'm afraid that we'll have two different kinds of
    'argument specifiers' (ie. QMP's and HMP's)

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

[Luiz Capitulino]

On Tue, 28 Sep 2010 12:08:07 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Jes.Sorensen@redhat.com writes:
> 
> > From: Jes Sorensen <Jes.Sorensen@redhat.com>
> >
> > Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
> > ---
> >  migration.c     |    4 ++--
> >  qemu-monitor.hx |    2 +-
> >  2 files changed, 3 insertions(+), 3 deletions(-)
> >
> > 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;
> 
> This isn't backwards bug-compatible.
> 
> Before, a client could send any number.  Any fractional part was
> ignored.
> 
> Now, the number must be an integer.  Other numbers are rejected.
> 
> I don't care myself, but others have argued most forcefully for keeping
> QMP fully backward compatible from 0.13 on, so they might object.

Can't we have a small fix for 0.13? We still have time I guess.

> 
> > diff --git a/qemu-monitor.hx b/qemu-monitor.hx
> > index 49bcd8d..7f58fb2 100644
> > --- a/qemu-monitor.hx
> > +++ b/qemu-monitor.hx
> > @@ -1093,7 +1093,7 @@ EQMP
> >  
> >      {
> >          .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,
> 
> Doesn't this change the interpretation of "42" from 42 to (42 << 20)?
> 

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

[Jes Sorensen]

On 09/28/10 12:08, Markus Armbruster wrote:
> Jes.Sorensen@redhat.com writes:
>> diff --git a/qemu-monitor.hx b/qemu-monitor.hx
>> index 49bcd8d..7f58fb2 100644
>> --- a/qemu-monitor.hx
>> +++ b/qemu-monitor.hx
>> @@ -1093,7 +1093,7 @@ EQMP
>>  
>>      {
>>          .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,
> 
> Doesn't this change the interpretation of "42" from 42 to (42 << 20)?

It was always so on the command line that a number without a specifier
meant MB. If the monitor defaulted to bytes it will get ugly to support
both defaults in common code.

Cheers,
Jes