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

[Qemu-devel] [PATCH v3 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.

v3: reject NaN and infinity in monitor (we shouldn't get them over
    QMP, because JSON doesn't support them)
    Bug fixes

v2: Initial post

v1: Never existed (I fat-fingered "PATCH v2" into my initial post)

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       |   81 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 qdict.c         |   24 ++++++++++++++++
 qdict.h         |    1 +
 qemu-monitor.hx |   10 ++++---
 6 files changed, 123 insertions(+), 36 deletions(-)

[Qemu-devel] [PATCH v3 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 ae3300b..a56d480 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 v3 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 |   24 ++++++++++++++++++++++++
 qdict.h |    1 +
 2 files changed, 25 insertions(+), 0 deletions(-)

diff --git a/qdict.c b/qdict.c
index ba8eef0..32119cf 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,29 @@ 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 or QInt object.
+ *
+ * Return number mapped by 'key'.
+ */
+double qdict_get_double(const QDict *qdict, const char *key)
+{
+    QObject *obj = qdict_get(qdict, key);
+
+    assert(obj);
+    switch (qobject_type(obj)) {
+    case QTYPE_QFLOAT:
+        return qfloat_get_double(qobject_to_qfloat(obj));
+    case QTYPE_QINT:
+        return qint_get_int(qobject_to_qint(obj));
+    default:
+        assert(0);
+    }
+}
+
+/**
  * 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 v3 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 |   64 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 64 insertions(+), 0 deletions(-)

diff --git a/monitor.c b/monitor.c
index a56d480..6325404 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 '/')
@@ -3180,6 +3185,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;
+
+    d = strtod(p, &tailp);
+    if (tailp == p) {
+        monitor_printf(mon, "Number expected\n");
+        return -1;
+    }
+    if (d != d || d - d != 0) {
+        /* NaN or infinity */
+        monitor_printf(mon, "Bad number\n");
+        return -1;
+    }
+    *pval = d;
+    *pp = tailp;
+    return 0;
+}
+
 static int get_str(char *buf, int buf_size, const char **pp)
 {
     const char *p;
@@ -3516,6 +3542,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;
@@ -3922,6 +3980,12 @@ static int check_arg(const CmdArgs *cmd_args, QDict *args)
                 return -1;
             }
             break;
+        case 'b':
+            if (qobject_type(value) != QTYPE_QINT && qobject_type(value) != QTYPE_QFLOAT) {
+                qemu_error_new(QERR_INVALID_PARAMETER_TYPE, name, "number");
+                return -1;
+            }
+            break;
         case '-':
             if (qobject_type(value) != QTYPE_QINT &&
                 qobject_type(value) != QTYPE_QBOOL) {
-- 
1.6.6

[Qemu-devel] [PATCH v3 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.

Negative arguments are no flushed to zero.  Before, they were cast to
uint32_t, which wrecked the sign.

Ridiculously large arguments including infinities are now rejected.
Before, they were interpreted as zero.  Same for NaN.

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..6abdc65 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(UINT32_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 v3 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 |    3 ++-
 3 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/migration.c b/migration.c
index 6abdc65..cd8d9e6 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..2354e4f 100644
--- a/qemu-monitor.hx
+++ b/qemu-monitor.hx
@@ -764,7 +764,8 @@ ETEXI
         .args_type  = "value:b",
         .params     = "value",
         .help       = "set maximum speed (in bytes) for migrations",
-        .mhandler.cmd = do_migrate_set_speed,
+        .user_print = monitor_user_noop,
+        .mhandler.cmd_new = do_migrate_set_speed,
     },
 
 STEXI
-- 
1.6.6

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

diff --git a/monitor.c b/monitor.c
index 6325404..a9d0c8a 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 '/')
@@ -3543,6 +3546,7 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon,
             }
             break;
         case 'b':
+        case 'T':
             {
                 double val;
 
@@ -3557,7 +3561,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;
@@ -3567,6 +3571,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;
@@ -3981,6 +3995,7 @@ static int check_arg(const CmdArgs *cmd_args, QDict *args)
             }
             break;
         case 'b':
+        case 'T':
             if (qobject_type(value) != QTYPE_QINT && qobject_type(value) != QTYPE_QFLOAT) {
                 qemu_error_new(QERR_INVALID_PARAMETER_TYPE, name, "number");
                 return -1;
-- 
1.6.6

[Qemu-devel] [PATCH v3 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.

Negative arguments are no flushed to zero.  Before, they were cast to
uint32_t, which wrecked the sign.

Ridiculously large arguments including infinities are now rejected.
Before, they were interpreted as zero.  Same for NaN.

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 cd8d9e6..7551953 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 2354e4f..9257d9c 100644
--- a/qemu-monitor.hx
+++ b/qemu-monitor.hx
@@ -775,7 +775,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 v3 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 |    3 ++-
 3 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/migration.c b/migration.c
index 7551953..5b28e42 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 9257d9c..213a2de 100644
--- a/qemu-monitor.hx
+++ b/qemu-monitor.hx
@@ -778,7 +778,8 @@ ETEXI
         .args_type  = "value:T",
         .params     = "value",
         .help       = "set maximum tolerated downtime (in seconds) for migrations",
-        .mhandler.cmd = do_migrate_set_downtime,
+        .user_print = monitor_user_noop,
+        .mhandler.cmd_new = do_migrate_set_downtime,
     },
 
 STEXI
-- 
1.6.6

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

[Luiz Capitulino]

On Mon, 25 Jan 2010 14:23:00 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> 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.

 Looks good to me.

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

[Anthony Liguori]

On 01/25/2010 07:23 AM, Markus Armbruster wrote:
> Helper function just like qdict_get_int(), just for QFloat/double.
>
> Signed-off-by: Markus Armbruster<armbru@redhat.com>
>    

Breaks check_qdict's build.  Need to update the Makefile.  You can just 
send a v4 of this patch.

Regards,

Anthony Liguori

[Qemu-devel] [PATCH v4 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>
---
 Makefile |    2 +-
 qdict.c  |   24 ++++++++++++++++++++++++
 qdict.h  |    1 +
 3 files changed, 26 insertions(+), 1 deletions(-)

diff --git a/Makefile b/Makefile
index 60d5c66..3f02c66 100644
--- a/Makefile
+++ b/Makefile
@@ -147,7 +147,7 @@ qemu-img-cmds.h: $(SRC_PATH)/qemu-img-cmds.hx
 
 check-qint: check-qint.o qint.o qemu-malloc.o
 check-qstring: check-qstring.o qstring.o qemu-malloc.o
-check-qdict: check-qdict.o qdict.o qint.o qstring.o qbool.o qemu-malloc.o qlist.o
+check-qdict: check-qdict.o qdict.o qfloat.o qint.o qstring.o qbool.o qemu-malloc.o qlist.o
 check-qlist: check-qlist.o qlist.o qint.o qemu-malloc.o
 check-qfloat: check-qfloat.o qfloat.o qemu-malloc.o
 check-qjson: check-qjson.o qfloat.o qint.o qdict.o qstring.o qlist.o qbool.o qjson.o json-streamer.o json-lexer.o json-parser.o qemu-malloc.o
diff --git a/qdict.c b/qdict.c
index ba8eef0..32119cf 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,29 @@ 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 or QInt object.
+ *
+ * Return number mapped by 'key'.
+ */
+double qdict_get_double(const QDict *qdict, const char *key)
+{
+    QObject *obj = qdict_get(qdict, key);
+
+    assert(obj);
+    switch (qobject_type(obj)) {
+    case QTYPE_QFLOAT:
+        return qfloat_get_double(qobject_to_qfloat(obj));
+    case QTYPE_QINT:
+        return qint_get_int(qobject_to_qint(obj));
+    default:
+        assert(0);
+    }
+}
+
+/**
  * 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

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

[Anthony Liguori]

On 01/25/2010 07:23 AM, Markus Armbruster wrote:
> Was forgotten in commit b6e098d7.
>
> Signed-off-by: Markus Armbruster<armbru@redhat.com>
>    

Applied all (including v4 of 2/8).  Thanks.

Regards,

Anthony Liguori

> ---
>   monitor.c |    2 ++
>   1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index ae3300b..a56d480 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 '/')
>