[Qemu-devel] [PATCH v2 0/8] Convert migrate_set_speed, migrate_set_downtime to QObject

[Qemu-devel] [PATCH v2 0/8] Convert migrate_set_speed, migrate_set_downtime to QObject

[Markus Armbruster]

These conversions take a few extra steps, because
do_migrate_set_speed() and do_migrate_set_downtime() interpret their
string argument as floating-point number + optional unit suffix.  This
is quite inappropriate for QMP.

Markus Armbruster (8):
  monitor: Document argument type 'M'
  QDict: New qdict_get_double()
  monitor: New argument type 'b'
  monitor: Use argument type 'b' for migrate_set_speed()
  monitor: convert do_migrate_set_speed() to QObject
  monitor: New argument type 'T'
  monitor: Use argument type 'T' for migrate_set_downtime()
  monitor: convert do_migrate_set_downtime() to QObject

 migration.c     |   38 ++++++----------------------
 migration.h     |    5 ++-
 monitor.c       |   74 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 qdict.c         |   15 +++++++++++
 qdict.h         |    1 +
 qemu-monitor.hx |    8 +++---
 6 files changed, 105 insertions(+), 36 deletions(-)

[Qemu-devel] [PATCH v2 1/8] monitor: Document argument type 'M'

[Markus Armbruster]

Was forgotten in commit b6e098d7.

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

diff --git a/monitor.c b/monitor.c
index b9166c3..775fe3f 100644
--- a/monitor.c
+++ b/monitor.c
@@ -68,6 +68,8 @@
  * 's'          string (accept optional quote)
  * 'i'          32 bit integer
  * 'l'          target long (32 or 64 bit)
+ * 'M'          just like 'l', except in user mode the value is
+ *              multiplied by 2^20 (think Mebibyte)
  * '/'          optional gdb-like print format (like "/10x")
  *
  * '?'          optional type (for all types, except '/')
-- 
1.6.6

[Qemu-devel] [PATCH v2 2/8] QDict: New qdict_get_double()

[Markus Armbruster]

Helper function just like qdict_get_int(), just for QFloat/double.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qdict.c |   15 +++++++++++++++
 qdict.h |    1 +
 2 files changed, 16 insertions(+), 0 deletions(-)

diff --git a/qdict.c b/qdict.c
index ba8eef0..de278b1 100644
--- a/qdict.c
+++ b/qdict.c
@@ -11,6 +11,7 @@
  */
 
 #include "qint.h"
+#include "qfloat.h"
 #include "qdict.h"
 #include "qbool.h"
 #include "qstring.h"
@@ -175,6 +176,20 @@ static QObject *qdict_get_obj(const QDict *qdict, const char *key,
 }
 
 /**
+ * qdict_get_double(): Get an number mapped by 'key'
+ *
+ * This function assumes that 'key' exists and it stores a
+ * QFloat object.
+ *
+ * Return number mapped by 'key'.
+ */
+double qdict_get_double(const QDict *qdict, const char *key)
+{
+    QObject *obj = qdict_get_obj(qdict, key, QTYPE_QFLOAT);
+    return qfloat_get_double(qobject_to_qfloat(obj));
+}
+
+/**
  * qdict_get_int(): Get an integer mapped by 'key'
  *
  * This function assumes that 'key' exists and it stores a
diff --git a/qdict.h b/qdict.h
index 5fef1ea..5649ccf 100644
--- a/qdict.h
+++ b/qdict.h
@@ -37,6 +37,7 @@ void qdict_iter(const QDict *qdict,
         qdict_put_obj(qdict, key, QOBJECT(obj))
 
 /* High level helpers */
+double qdict_get_double(const QDict *qdict, const char *key);
 int64_t qdict_get_int(const QDict *qdict, const char *key);
 int qdict_get_bool(const QDict *qdict, const char *key);
 QList *qdict_get_qlist(const QDict *qdict, const char *key);
-- 
1.6.6

[Qemu-devel] [PATCH v2 3/8] monitor: New argument type 'b'

[Markus Armbruster]

This is a double value with optional suffixes G, g, M, m, K, k.  We'll
need this to get migrate_set_speed() QMP-ready.

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

diff --git a/monitor.c b/monitor.c
index 775fe3f..ce97e7b 100644
--- a/monitor.c
+++ b/monitor.c
@@ -47,6 +47,7 @@
 #include "kvm.h"
 #include "acl.h"
 #include "qint.h"
+#include "qfloat.h"
 #include "qlist.h"
 #include "qdict.h"
 #include "qbool.h"
@@ -70,6 +71,10 @@
  * 'l'          target long (32 or 64 bit)
  * 'M'          just like 'l', except in user mode the value is
  *              multiplied by 2^20 (think Mebibyte)
+ * 'b'          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
  * '/'          optional gdb-like print format (like "/10x")
  *
  * '?'          optional type (for all types, except '/')
@@ -3181,6 +3186,27 @@ static int get_expr(Monitor *mon, int64_t *pval, const char **pp)
     return 0;
 }
 
+static int get_double(Monitor *mon, double *pval, const char **pp)
+{
+    const char *p = *pp;
+    char *tailp;
+    double d;
+
+    errno = 0;
+    d = strtod(p, &tailp);
+    if (tailp == p) {
+        monitor_printf(mon, "Number expected\n");
+        return -1;
+    }
+    if (errno) {
+        monitor_printf(mon, "Bad number (%s)\n", strerror(errno));
+        return -1;
+    }
+    *pval = d;
+    *pp = tailp;
+    return 0;
+}
+
 static int get_str(char *buf, int buf_size, const char **pp)
 {
     const char *p;
@@ -3517,6 +3543,38 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon,
                 qdict_put(qdict, key, qint_from_int(val));
             }
             break;
+        case 'b':
+            {
+                double val;
+
+                while (qemu_isspace(*p))
+                    p++;
+                if (*typestr == '?') {
+                    typestr++;
+                    if (*p == '\0') {
+                        break;
+                    }
+                }
+                if (get_double(mon, &val, &p) < 0) {
+                    goto fail;
+                }
+                if (*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 (*p && !qemu_isspace(*p)) {
+                    monitor_printf(mon, "Unknown unit suffix\n");
+                    goto fail;
+                }
+                qdict_put(qdict, key, qfloat_from_double(val));
+            }
+            break;
         case '-':
             {
                 const char *tmp = p;
-- 
1.6.6

[Qemu-devel] [PATCH v2 4/8] monitor: Use argument type 'b' for migrate_set_speed()

[Markus Armbruster]

Before, it used type 's', which strips quotes and interprets escapes,
and is quite inappropriate for QMP.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 migration.c     |   18 +++---------------
 qemu-monitor.hx |    2 +-
 2 files changed, 4 insertions(+), 16 deletions(-)

diff --git a/migration.c b/migration.c
index 598f8df..4e7a1ea 100644
--- a/migration.c
+++ b/migration.c
@@ -109,23 +109,11 @@ void do_migrate_cancel(Monitor *mon, const QDict *qdict, QObject **ret_data)
 void do_migrate_set_speed(Monitor *mon, const QDict *qdict)
 {
     double d;
-    char *ptr;
     FdMigrationState *s;
-    const char *value = qdict_get_str(qdict, "value");
-
-    d = strtod(value, &ptr);
-    switch (*ptr) {
-    case 'G': case 'g':
-        d *= 1024;
-    case 'M': case 'm':
-        d *= 1024;
-    case 'K': case 'k':
-        d *= 1024;
-    default:
-        break;
-    }
 
-    max_throttle = (uint32_t)d;
+    d = qdict_get_double(qdict, "value");
+    d = MAX(0, MIN(INT32_MAX, d));
+    max_throttle = d;
 
     s = migrate_to_fms(current_migration);
     if (s && s->file) {
diff --git a/qemu-monitor.hx b/qemu-monitor.hx
index 415734a..9d2359d 100644
--- a/qemu-monitor.hx
+++ b/qemu-monitor.hx
@@ -761,7 +761,7 @@ ETEXI
 
     {
         .name       = "migrate_set_speed",
-        .args_type  = "value:s",
+        .args_type  = "value:b",
         .params     = "value",
         .help       = "set maximum speed (in bytes) for migrations",
         .mhandler.cmd = do_migrate_set_speed,
-- 
1.6.6

[Qemu-devel] [PATCH v2 5/8] monitor: convert do_migrate_set_speed() to QObject

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 migration.c     |    2 +-
 migration.h     |    2 +-
 qemu-monitor.hx |    2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/migration.c b/migration.c
index 4e7a1ea..e8802ba 100644
--- a/migration.c
+++ b/migration.c
@@ -106,7 +106,7 @@ void do_migrate_cancel(Monitor *mon, const QDict *qdict, QObject **ret_data)
         s->cancel(s);
 }
 
-void do_migrate_set_speed(Monitor *mon, const QDict *qdict)
+void do_migrate_set_speed(Monitor *mon, const QDict *qdict, QObject **ret_data)
 {
     double d;
     FdMigrationState *s;
diff --git a/migration.h b/migration.h
index cbd456b..3ac208b 100644
--- a/migration.h
+++ b/migration.h
@@ -56,7 +56,7 @@ void do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data);
 
 void do_migrate_cancel(Monitor *mon, const QDict *qdict, QObject **ret_data);
 
-void do_migrate_set_speed(Monitor *mon, const QDict *qdict);
+void do_migrate_set_speed(Monitor *mon, const QDict *qdict, QObject **ret_data);
 
 uint64_t migrate_max_downtime(void);
 
diff --git a/qemu-monitor.hx b/qemu-monitor.hx
index 9d2359d..20c356a 100644
--- a/qemu-monitor.hx
+++ b/qemu-monitor.hx
@@ -764,7 +764,7 @@ ETEXI
         .args_type  = "value:b",
         .params     = "value",
         .help       = "set maximum speed (in bytes) for migrations",
-        .mhandler.cmd = do_migrate_set_speed,
+        .mhandler.cmd_new = do_migrate_set_speed,
     },
 
 STEXI
-- 
1.6.6

[Qemu-devel] [PATCH v2 6/8] monitor: New argument type 'T'

[Markus Armbruster]

This is a double value with optional suffixes ms, us, ns.  We'll need
this to get migrate_set_downtime() QMP-ready.

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

diff --git a/monitor.c b/monitor.c
index ce97e7b..6dafe0b 100644
--- a/monitor.c
+++ b/monitor.c
@@ -75,6 +75,9 @@
  *              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
  * '/'          optional gdb-like print format (like "/10x")
  *
  * '?'          optional type (for all types, except '/')
@@ -3544,6 +3547,7 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon,
             }
             break;
         case 'b':
+        case 'T':
             {
                 double val;
 
@@ -3558,7 +3562,7 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon,
                 if (get_double(mon, &val, &p) < 0) {
                     goto fail;
                 }
-                if (*p) {
+                if (c == 'b' && *p) {
                     switch (*p) {
                     case 'K': case 'k':
                         val *= 1 << 10; p++; break;
@@ -3568,6 +3572,16 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon,
                         val *= 1 << 30; p++; break;
                     }
                 }
+                if (c == 'T' && p[0] && p[1] == 's') {
+                    switch (*p) {
+                    case 'm':
+                        val /= 1e3; p += 2; break;
+                    case 'u':
+                        val /= 1e6; p += 2; break;
+                    case 'n':
+                        val /= 1e9; p += 2; break;
+                    }
+                }
                 if (*p && !qemu_isspace(*p)) {
                     monitor_printf(mon, "Unknown unit suffix\n");
                     goto fail;
-- 
1.6.6

[Qemu-devel] [PATCH v2 7/8] monitor: Use argument type 'T' for migrate_set_downtime()

[Markus Armbruster]

Before, it used type 's', which strips quotes and interprets escapes,
and is quite inappropriate for QMP.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 migration.c     |   15 ++-------------
 qemu-monitor.hx |    2 +-
 2 files changed, 3 insertions(+), 14 deletions(-)

diff --git a/migration.c b/migration.c
index e8802ba..c7763f6 100644
--- a/migration.c
+++ b/migration.c
@@ -134,21 +134,10 @@ uint64_t migrate_max_downtime(void)
 
 void do_migrate_set_downtime(Monitor *mon, const QDict *qdict)
 {
-    char *ptr;
     double d;
-    const char *value = qdict_get_str(qdict, "value");
-
-    d = strtod(value, &ptr);
-    if (!strcmp(ptr,"ms")) {
-        d *= 1000000;
-    } else if (!strcmp(ptr,"us")) {
-        d *= 1000;
-    } else if (!strcmp(ptr,"ns")) {
-    } else {
-        /* all else considered to be seconds */
-        d *= 1000000000;
-    }
 
+    d = qdict_get_double(qdict, "value") / 1e9;
+    d = MAX(0, MIN(UINT64_MAX, d));
     max_downtime = (uint64_t)d;
 }
 
diff --git a/qemu-monitor.hx b/qemu-monitor.hx
index 20c356a..61b99ba 100644
--- a/qemu-monitor.hx
+++ b/qemu-monitor.hx
@@ -774,7 +774,7 @@ ETEXI
 
     {
         .name       = "migrate_set_downtime",
-        .args_type  = "value:s",
+        .args_type  = "value:T",
         .params     = "value",
         .help       = "set maximum tolerated downtime (in seconds) for migrations",
         .mhandler.cmd = do_migrate_set_downtime,
-- 
1.6.6

[Qemu-devel] [PATCH v2 8/8] monitor: convert do_migrate_set_downtime() to QObject

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 migration.c     |    3 ++-
 migration.h     |    3 ++-
 qemu-monitor.hx |    2 +-
 3 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/migration.c b/migration.c
index c7763f6..2dfb4cd 100644
--- a/migration.c
+++ b/migration.c
@@ -132,7 +132,8 @@ uint64_t migrate_max_downtime(void)
     return max_downtime;
 }
 
-void do_migrate_set_downtime(Monitor *mon, const QDict *qdict)
+void do_migrate_set_downtime(Monitor *mon, const QDict *qdict,
+                             QObject **ret_data)
 {
     double d;
 
diff --git a/migration.h b/migration.h
index 3ac208b..65572c1 100644
--- a/migration.h
+++ b/migration.h
@@ -60,7 +60,8 @@ void do_migrate_set_speed(Monitor *mon, const QDict *qdict, QObject **ret_data);
 
 uint64_t migrate_max_downtime(void);
 
-void do_migrate_set_downtime(Monitor *mon, const QDict *qdict);
+void do_migrate_set_downtime(Monitor *mon, const QDict *qdict,
+                             QObject **ret_data);
 
 void do_info_migrate_print(Monitor *mon, const QObject *data);
 
diff --git a/qemu-monitor.hx b/qemu-monitor.hx
index 61b99ba..28654f5 100644
--- a/qemu-monitor.hx
+++ b/qemu-monitor.hx
@@ -777,7 +777,7 @@ ETEXI
         .args_type  = "value:T",
         .params     = "value",
         .help       = "set maximum tolerated downtime (in seconds) for migrations",
-        .mhandler.cmd = do_migrate_set_downtime,
+        .mhandler.cmd_new = do_migrate_set_downtime,
     },
 
 STEXI
-- 
1.6.6

[Qemu-devel] Re: [PATCH v2 0/8] Convert migrate_set_speed, migrate_set_downtime to QObject

[Markus Armbruster]

Please ignore the v2 in the subject, it's an accident.

RE: [Qemu-devel] [PATCH v2 1/8] monitor: Document argument type 'M'

[Krumme, Chris]

> -----Original Message-----
> From: 
> qemu-devel-bounces+chris.krumme=windriver.com@nongnu.org 
> [mailto:qemu-devel-bounces+chris.krumme=windriver.com@nongnu.o
> rg] On Behalf Of Markus Armbruster
> Sent: Wednesday, January 20, 2010 10:08 AM
> To: qemu-devel@nongnu.org
> Subject: [Qemu-devel] [PATCH v2 1/8] monitor: Document 
> argument type 'M'
> 
> Was forgotten in commit b6e098d7.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  monitor.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/monitor.c b/monitor.c
> index b9166c3..775fe3f 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -68,6 +68,8 @@
>   * 's'          string (accept optional quote)
>   * 'i'          32 bit integer
>   * 'l'          target long (32 or 64 bit)
> + * 'M'          just like 'l', except in user mode the value is
> + *              multiplied by 2^20 (think Mebibyte)


Hello Markus,

Not sure of the best answer, but thought there should be some
discussion.

You mention Mebibyte, which according to the all knowing Wikipedia, is
abbreviated Mi.

I understand that if you will only support one, then maybe you don't
need to differentiate from Megabyte, but then in a later patch you use
m, u, and n for powers of 10. This causes your new double format to use
powers of 2 above one and powers of 10 below.

Maybe this is just one of those geek things, powers of 2 for integers,
and powers of 10 for fractions.

Good luck.

Chris

>   * '/'          optional gdb-like print format (like "/10x")
>   *
>   * '?'          optional type (for all types, except '/')
> -- 
> 1.6.6
> 
> 
> 
> 

Re: [Qemu-devel] [PATCH v2 1/8] monitor: Document argument type 'M'

[Markus Armbruster]

"Krumme, Chris" <Chris.Krumme@windriver.com> writes:

> Hello Markus,
>
> Not sure of the best answer, but thought there should be some
> discussion.
>
> You mention Mebibyte, which according to the all knowing Wikipedia, is
> abbreviated Mi.
>
> I understand that if you will only support one, then maybe you don't
> need to differentiate from Megabyte, but then in a later patch you use
> m, u, and n for powers of 10. This causes your new double format to use
> powers of 2 above one and powers of 10 below.

Really?

Argument types M and b use powers of 2.

Argument type T uses powers of 10.

> Maybe this is just one of those geek things, powers of 2 for integers,
> and powers of 10 for fractions.
>
> Good luck.

I merely reimplemented existing usage in a way that does the right thing
for QMP.  Can't say I like the ad-hocery there, but improving the
(non-QMP) monitor is not my objective right now.

Re: [Qemu-devel] [PATCH v2 3/8] monitor: New argument type 'b'

[Luiz Capitulino]

On Wed, 20 Jan 2010 17:08:17 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> This is a double value with optional suffixes G, g, M, m, K, k.  We'll
> need this to get migrate_set_speed() QMP-ready.

 Nice, not only good for QMP: we're moving this kind of handling
from the handlers to common code, which is the right thing to do.

 The only possible issue is that, if we decide to move all this stuff
to json, such types will make the change complex. But that's something
for the future.

 Some comments follow.

> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  monitor.c |   58 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 58 insertions(+), 0 deletions(-)
> 
> diff --git a/monitor.c b/monitor.c
> index 775fe3f..ce97e7b 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -47,6 +47,7 @@
>  #include "kvm.h"
>  #include "acl.h"
>  #include "qint.h"
> +#include "qfloat.h"
>  #include "qlist.h"
>  #include "qdict.h"
>  #include "qbool.h"
> @@ -70,6 +71,10 @@
>   * 'l'          target long (32 or 64 bit)
>   * 'M'          just like 'l', except in user mode the value is
>   *              multiplied by 2^20 (think Mebibyte)
> + * 'b'          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
>   * '/'          optional gdb-like print format (like "/10x")
>   *
>   * '?'          optional type (for all types, except '/')
> @@ -3181,6 +3186,27 @@ static int get_expr(Monitor *mon, int64_t *pval, const char **pp)
>      return 0;
>  }
>  
> +static int get_double(Monitor *mon, double *pval, const char **pp)
> +{
> +    const char *p = *pp;
> +    char *tailp;

 Better to init to NULL?

> +    double d;
> +
> +    errno = 0;
> +    d = strtod(p, &tailp);
> +    if (tailp == p) {
> +        monitor_printf(mon, "Number expected\n");
> +        return -1;
> +    }
> +    if (errno) {
> +        monitor_printf(mon, "Bad number (%s)\n", strerror(errno));
> +        return -1;
> +    }

 Should we trust errno this way? The manpage only mentions ERANGE.

> +    *pval = d;
> +    *pp = tailp;
> +    return 0;
> +}
> +
>  static int get_str(char *buf, int buf_size, const char **pp)
>  {
>      const char *p;
> @@ -3517,6 +3543,38 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon,
>                  qdict_put(qdict, key, qint_from_int(val));
>              }
>              break;
> +        case 'b':
> +            {
> +                double val;
> +
> +                while (qemu_isspace(*p))
> +                    p++;
> +                if (*typestr == '?') {
> +                    typestr++;
> +                    if (*p == '\0') {
> +                        break;
> +                    }
> +                }
> +                if (get_double(mon, &val, &p) < 0) {
> +                    goto fail;
> +                }
> +                if (*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 (*p && !qemu_isspace(*p)) {
> +                    monitor_printf(mon, "Unknown unit suffix\n");
> +                    goto fail;
> +                }

 A good way to test if 'p' handling is correct, is to write a test
handler which has different types (say, 'foo:b,str:s,bla:i') and print
the values to see if they match what we expect or have hardcoded
to values in a specific test handler...

> +                qdict_put(qdict, key, qfloat_from_double(val));
> +            }
> +            break;
>          case '-':
>              {
>                  const char *tmp = p;

Re: [Qemu-devel] [PATCH v2 6/8] monitor: New argument type 'T'

[Luiz Capitulino]

On Wed, 20 Jan 2010 17:08:20 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> This is a double value with optional suffixes ms, us, ns.  We'll need
> this to get migrate_set_downtime() QMP-ready.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  monitor.c |   16 +++++++++++++++-
>  1 files changed, 15 insertions(+), 1 deletions(-)
> 
> diff --git a/monitor.c b/monitor.c
> index ce97e7b..6dafe0b 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -75,6 +75,9 @@
>   *              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
>   * '/'          optional gdb-like print format (like "/10x")
>   *
>   * '?'          optional type (for all types, except '/')
> @@ -3544,6 +3547,7 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon,
>              }
>              break;
>          case 'b':
> +        case 'T':
>              {
>                  double val;
>  
> @@ -3558,7 +3562,7 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon,
>                  if (get_double(mon, &val, &p) < 0) {
>                      goto fail;
>                  }
> -                if (*p) {
> +                if (c == 'b' && *p) {
>                      switch (*p) {
>                      case 'K': case 'k':
>                          val *= 1 << 10; p++; break;
> @@ -3568,6 +3572,16 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon,
>                          val *= 1 << 30; p++; break;
>                      }
>                  }
> +                if (c == 'T' && p[0] && p[1] == 's') {

 Is this indexing of 'p' really correct? What if the value you're interested
is at the of the string? Like:

.args_type = "str:s,value:b"

> +                    switch (*p) {
> +                    case 'm':
> +                        val /= 1e3; p += 2; break;
> +                    case 'u':
> +                        val /= 1e6; p += 2; break;
> +                    case 'n':
> +                        val /= 1e9; p += 2; break;
> +                    }
> +                }
>                  if (*p && !qemu_isspace(*p)) {
>                      monitor_printf(mon, "Unknown unit suffix\n");
>                      goto fail;

Re: [Qemu-devel] [PATCH v2 6/8] monitor: New argument type 'T'

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Wed, 20 Jan 2010 17:08:20 +0100
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> This is a double value with optional suffixes ms, us, ns.  We'll need
>> this to get migrate_set_downtime() QMP-ready.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  monitor.c |   16 +++++++++++++++-
>>  1 files changed, 15 insertions(+), 1 deletions(-)
>> 
>> diff --git a/monitor.c b/monitor.c
>> index ce97e7b..6dafe0b 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -75,6 +75,9 @@
>>   *              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
>>   * '/'          optional gdb-like print format (like "/10x")
>>   *
>>   * '?'          optional type (for all types, except '/')
>> @@ -3544,6 +3547,7 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon,
>>              }
>>              break;
>>          case 'b':
>> +        case 'T':
>>              {
>>                  double val;
>>  
>> @@ -3558,7 +3562,7 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon,
>>                  if (get_double(mon, &val, &p) < 0) {
>>                      goto fail;
>>                  }
>> -                if (*p) {
>> +                if (c == 'b' && *p) {
>>                      switch (*p) {
>>                      case 'K': case 'k':
>>                          val *= 1 << 10; p++; break;
>> @@ -3568,6 +3572,16 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon,
>>                          val *= 1 << 30; p++; break;
>>                      }
>>                  }
>> +                if (c == 'T' && p[0] && p[1] == 's') {
>
>  Is this indexing of 'p' really correct? What if the value you're interested
> is at the of the string? Like:
>
> .args_type = "str:s,value:b"

p points right behind the floating-point number.  If no characters
follow, it points to the string's terminating zero.  If exactly one
character follows, it points to that one, and the next one is the
terminating zero.  If more than one follow, p points to the first one.

"p[0] && p[1] == 's'" is safe, because p[0] is safe, and if p[0] != 0,
then p[1] is also safe.

Convinced?

>> +                    switch (*p) {
>> +                    case 'm':
>> +                        val /= 1e3; p += 2; break;
>> +                    case 'u':
>> +                        val /= 1e6; p += 2; break;
>> +                    case 'n':
>> +                        val /= 1e9; p += 2; break;
>> +                    }
>> +                }
>>                  if (*p && !qemu_isspace(*p)) {
>>                      monitor_printf(mon, "Unknown unit suffix\n");
>>                      goto fail;

Re: [Qemu-devel] [PATCH v2 3/8] monitor: New argument type 'b'

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Wed, 20 Jan 2010 17:08:17 +0100
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> This is a double value with optional suffixes G, g, M, m, K, k.  We'll
>> need this to get migrate_set_speed() QMP-ready.
>
>  Nice, not only good for QMP: we're moving this kind of handling
> from the handlers to common code, which is the right thing to do.
>
>  The only possible issue is that, if we decide to move all this stuff
> to json, such types will make the change complex. But that's something
> for the future.
>
>  Some comments follow.
>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  monitor.c |   58 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 files changed, 58 insertions(+), 0 deletions(-)
>> 
>> diff --git a/monitor.c b/monitor.c
>> index 775fe3f..ce97e7b 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -47,6 +47,7 @@
>>  #include "kvm.h"
>>  #include "acl.h"
>>  #include "qint.h"
>> +#include "qfloat.h"
>>  #include "qlist.h"
>>  #include "qdict.h"
>>  #include "qbool.h"
>> @@ -70,6 +71,10 @@
>>   * 'l'          target long (32 or 64 bit)
>>   * 'M'          just like 'l', except in user mode the value is
>>   *              multiplied by 2^20 (think Mebibyte)
>> + * 'b'          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
>>   * '/'          optional gdb-like print format (like "/10x")
>>   *
>>   * '?'          optional type (for all types, except '/')
>> @@ -3181,6 +3186,27 @@ static int get_expr(Monitor *mon, int64_t *pval, const char **pp)
>>      return 0;
>>  }
>>  
>> +static int get_double(Monitor *mon, double *pval, const char **pp)
>> +{
>> +    const char *p = *pp;
>> +    char *tailp;
>
>  Better to init to NULL?

Not necessary, as strtod() sets tailp unconditionally.

>> +    double d;
>> +
>> +    errno = 0;
>> +    d = strtod(p, &tailp);
>> +    if (tailp == p) {
>> +        monitor_printf(mon, "Number expected\n");
>> +        return -1;
>> +    }
>> +    if (errno) {
>> +        monitor_printf(mon, "Bad number (%s)\n", strerror(errno));
>> +        return -1;
>> +    }
>
>  Should we trust errno this way? The manpage only mentions ERANGE.

Unless we want to ignore errors other than the "Number expected" caught
above, we have to check errno.  strtod() doesn't have a distinct error
value.

I'm not particular about reporting strerror(errno).

>> +    *pval = d;
>> +    *pp = tailp;
>> +    return 0;
>> +}
>> +
>>  static int get_str(char *buf, int buf_size, const char **pp)
>>  {
>>      const char *p;
>> @@ -3517,6 +3543,38 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon,
>>                  qdict_put(qdict, key, qint_from_int(val));
>>              }
>>              break;
>> +        case 'b':
>> +            {
>> +                double val;
>> +
>> +                while (qemu_isspace(*p))
>> +                    p++;
>> +                if (*typestr == '?') {
>> +                    typestr++;
>> +                    if (*p == '\0') {
>> +                        break;
>> +                    }
>> +                }
>> +                if (get_double(mon, &val, &p) < 0) {
>> +                    goto fail;
>> +                }
>> +                if (*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 (*p && !qemu_isspace(*p)) {
>> +                    monitor_printf(mon, "Unknown unit suffix\n");
>> +                    goto fail;
>> +                }
>
>  A good way to test if 'p' handling is correct, is to write a test
> handler which has different types (say, 'foo:b,str:s,bla:i') and print
> the values to see if they match what we expect or have hardcoded
> to values in a specific test handler...

Umm, what do you want me to do here?

>> +                qdict_put(qdict, key, qfloat_from_double(val));
>> +            }
>> +            break;
>>          case '-':
>>              {
>>                  const char *tmp = p;

Re: [Qemu-devel] [PATCH v2 6/8] monitor: New argument type 'T'

[Luiz Capitulino]

On Thu, 21 Jan 2010 14:54:51 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > On Wed, 20 Jan 2010 17:08:20 +0100
> > Markus Armbruster <armbru@redhat.com> wrote:
> >
> >> This is a double value with optional suffixes ms, us, ns.  We'll need
> >> this to get migrate_set_downtime() QMP-ready.
> >> 
> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> >> ---
> >>  monitor.c |   16 +++++++++++++++-
> >>  1 files changed, 15 insertions(+), 1 deletions(-)
> >> 
> >> diff --git a/monitor.c b/monitor.c
> >> index ce97e7b..6dafe0b 100644
> >> --- a/monitor.c
> >> +++ b/monitor.c
> >> @@ -75,6 +75,9 @@
> >>   *              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
> >>   * '/'          optional gdb-like print format (like "/10x")
> >>   *
> >>   * '?'          optional type (for all types, except '/')
> >> @@ -3544,6 +3547,7 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon,
> >>              }
> >>              break;
> >>          case 'b':
> >> +        case 'T':
> >>              {
> >>                  double val;
> >>  
> >> @@ -3558,7 +3562,7 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon,
> >>                  if (get_double(mon, &val, &p) < 0) {
> >>                      goto fail;
> >>                  }
> >> -                if (*p) {
> >> +                if (c == 'b' && *p) {
> >>                      switch (*p) {
> >>                      case 'K': case 'k':
> >>                          val *= 1 << 10; p++; break;
> >> @@ -3568,6 +3572,16 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon,
> >>                          val *= 1 << 30; p++; break;
> >>                      }
> >>                  }
> >> +                if (c == 'T' && p[0] && p[1] == 's') {
> >
> >  Is this indexing of 'p' really correct? What if the value you're interested
> > is at the of the string? Like:
> >
> > .args_type = "str:s,value:b"
> 
> p points right behind the floating-point number.  If no characters
> follow, it points to the string's terminating zero.  If exactly one
> character follows, it points to that one, and the next one is the
> terminating zero.  If more than one follow, p points to the first one.
> 
> "p[0] && p[1] == 's'" is safe, because p[0] is safe, and if p[0] != 0,
> then p[1] is also safe.
> 
> Convinced?

 Yes, looks fine.

> 
> >> +                    switch (*p) {
> >> +                    case 'm':
> >> +                        val /= 1e3; p += 2; break;
> >> +                    case 'u':
> >> +                        val /= 1e6; p += 2; break;
> >> +                    case 'n':
> >> +                        val /= 1e9; p += 2; break;
> >> +                    }
> >> +                }
> >>                  if (*p && !qemu_isspace(*p)) {
> >>                      monitor_printf(mon, "Unknown unit suffix\n");
> >>                      goto fail;

Re: [Qemu-devel] [PATCH v2 3/8] monitor: New argument type 'b'

[Luiz Capitulino]

On Thu, 21 Jan 2010 15:04:43 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > On Wed, 20 Jan 2010 17:08:17 +0100
> > Markus Armbruster <armbru@redhat.com> wrote:
> >
> >> This is a double value with optional suffixes G, g, M, m, K, k.  We'll
> >> need this to get migrate_set_speed() QMP-ready.
> >
> >  Nice, not only good for QMP: we're moving this kind of handling
> > from the handlers to common code, which is the right thing to do.
> >
> >  The only possible issue is that, if we decide to move all this stuff
> > to json, such types will make the change complex. But that's something
> > for the future.
> >
> >  Some comments follow.
> >
> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> >> ---
> >>  monitor.c |   58 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>  1 files changed, 58 insertions(+), 0 deletions(-)
> >> 
> >> diff --git a/monitor.c b/monitor.c
> >> index 775fe3f..ce97e7b 100644
> >> --- a/monitor.c
> >> +++ b/monitor.c
> >> @@ -47,6 +47,7 @@
> >>  #include "kvm.h"
> >>  #include "acl.h"
> >>  #include "qint.h"
> >> +#include "qfloat.h"
> >>  #include "qlist.h"
> >>  #include "qdict.h"
> >>  #include "qbool.h"
> >> @@ -70,6 +71,10 @@
> >>   * 'l'          target long (32 or 64 bit)
> >>   * 'M'          just like 'l', except in user mode the value is
> >>   *              multiplied by 2^20 (think Mebibyte)
> >> + * 'b'          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
> >>   * '/'          optional gdb-like print format (like "/10x")
> >>   *
> >>   * '?'          optional type (for all types, except '/')
> >> @@ -3181,6 +3186,27 @@ static int get_expr(Monitor *mon, int64_t *pval, const char **pp)
> >>      return 0;
> >>  }
> >>  
> >> +static int get_double(Monitor *mon, double *pval, const char **pp)
> >> +{
> >> +    const char *p = *pp;
> >> +    char *tailp;
> >
> >  Better to init to NULL?
> 
> Not necessary, as strtod() sets tailp unconditionally.

 Ok.

> >> +    double d;
> >> +
> >> +    errno = 0;
> >> +    d = strtod(p, &tailp);
> >> +    if (tailp == p) {
> >> +        monitor_printf(mon, "Number expected\n");
> >> +        return -1;
> >> +    }
> >> +    if (errno) {
> >> +        monitor_printf(mon, "Bad number (%s)\n", strerror(errno));
> >> +        return -1;
> >> +    }
> >
> >  Should we trust errno this way? The manpage only mentions ERANGE.
> 
> Unless we want to ignore errors other than the "Number expected" caught
> above, we have to check errno.  strtod() doesn't have a distinct error
> value.
> 
> I'm not particular about reporting strerror(errno).

 Ok, no big deal. I just tend to do strictly what the manpages says.

> >> +    *pval = d;
> >> +    *pp = tailp;
> >> +    return 0;
> >> +}
> >> +
> >>  static int get_str(char *buf, int buf_size, const char **pp)
> >>  {
> >>      const char *p;
> >> @@ -3517,6 +3543,38 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon,
> >>                  qdict_put(qdict, key, qint_from_int(val));
> >>              }
> >>              break;
> >> +        case 'b':
> >> +            {
> >> +                double val;
> >> +
> >> +                while (qemu_isspace(*p))
> >> +                    p++;
> >> +                if (*typestr == '?') {
> >> +                    typestr++;
> >> +                    if (*p == '\0') {
> >> +                        break;
> >> +                    }
> >> +                }
> >> +                if (get_double(mon, &val, &p) < 0) {
> >> +                    goto fail;
> >> +                }
> >> +                if (*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 (*p && !qemu_isspace(*p)) {
> >> +                    monitor_printf(mon, "Unknown unit suffix\n");
> >> +                    goto fail;
> >> +                }
> >
> >  A good way to test if 'p' handling is correct, is to write a test
> > handler which has different types (say, 'foo:b,str:s,bla:i') and print
> > the values to see if they match what we expect or have hardcoded
> > to values in a specific test handler...
> 
> Umm, what do you want me to do here?

 Just a suggestion on how I would test this stuff.

> 
> >> +                qdict_put(qdict, key, qfloat_from_double(val));
> >> +            }
> >> +            break;
> >>          case '-':
> >>              {
> >>                  const char *tmp = p;

Re: [Qemu-devel] [PATCH v2 3/8] monitor: New argument type 'b'

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Thu, 21 Jan 2010 15:04:43 +0100
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Luiz Capitulino <lcapitulino@redhat.com> writes:
>> 
>> > On Wed, 20 Jan 2010 17:08:17 +0100
>> > Markus Armbruster <armbru@redhat.com> wrote:
>> >
>> >> This is a double value with optional suffixes G, g, M, m, K, k.  We'll
>> >> need this to get migrate_set_speed() QMP-ready.
>> >
>> >  Nice, not only good for QMP: we're moving this kind of handling
>> > from the handlers to common code, which is the right thing to do.
>> >
>> >  The only possible issue is that, if we decide to move all this stuff
>> > to json, such types will make the change complex. But that's something
>> > for the future.
>> >
>> >  Some comments follow.
>> >
>> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> >> ---
>> >>  monitor.c |   58 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> >>  1 files changed, 58 insertions(+), 0 deletions(-)
>> >> 
>> >> diff --git a/monitor.c b/monitor.c
>> >> index 775fe3f..ce97e7b 100644
>> >> --- a/monitor.c
>> >> +++ b/monitor.c
>> >> @@ -47,6 +47,7 @@
>> >>  #include "kvm.h"
>> >>  #include "acl.h"
>> >>  #include "qint.h"
>> >> +#include "qfloat.h"
>> >>  #include "qlist.h"
>> >>  #include "qdict.h"
>> >>  #include "qbool.h"
>> >> @@ -70,6 +71,10 @@
>> >>   * 'l'          target long (32 or 64 bit)
>> >>   * 'M'          just like 'l', except in user mode the value is
>> >>   *              multiplied by 2^20 (think Mebibyte)
>> >> + * 'b'          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
>> >>   * '/'          optional gdb-like print format (like "/10x")
>> >>   *
>> >>   * '?'          optional type (for all types, except '/')
>> >> @@ -3181,6 +3186,27 @@ static int get_expr(Monitor *mon, int64_t *pval, const char **pp)
>> >>      return 0;
>> >>  }
>> >>  
>> >> +static int get_double(Monitor *mon, double *pval, const char **pp)
>> >> +{
>> >> +    const char *p = *pp;
>> >> +    char *tailp;
>> >
>> >  Better to init to NULL?
>> 
>> Not necessary, as strtod() sets tailp unconditionally.
>
>  Ok.
>
>> >> +    double d;
>> >> +
>> >> +    errno = 0;
>> >> +    d = strtod(p, &tailp);
>> >> +    if (tailp == p) {
>> >> +        monitor_printf(mon, "Number expected\n");
>> >> +        return -1;
>> >> +    }
>> >> +    if (errno) {
>> >> +        monitor_printf(mon, "Bad number (%s)\n", strerror(errno));
>> >> +        return -1;
>> >> +    }
>> >
>> >  Should we trust errno this way? The manpage only mentions ERANGE.
>> 
>> Unless we want to ignore errors other than the "Number expected" caught
>> above, we have to check errno.  strtod() doesn't have a distinct error
>> value.
>> 
>> I'm not particular about reporting strerror(errno).
>
>  Ok, no big deal. I just tend to do strictly what the manpages says.

We might want to ignore underflow silently, and fail loudly for ininity
and NaN.  Opinions?

[...]

[Qemu-devel] Re: [PATCH v2 0/8] Convert migrate_set_speed, migrate_set_downtime to QObject

[Markus Armbruster]

I'll respin this series.