[Qemu-devel] [PATCH 0/4] monitor: Convert do_set_link() to QObject, QError

[Qemu-devel] [PATCH 0/4] monitor: Convert do_set_link() to QObject, QError

[Markus Armbruster]

PATCH 3/4 changes syntax of set_link's second argument from up|down to
on|off.  I feel that the argument needs to be boolean in QMP, and this
is the simplest way to get it.

Alternatives I could try if the syntax change is unwanted:

* Use the old string argument in QMP.  Easy.

* Don't convert set_link, create a new command with a boolean
  argument.

* Create a argument parser for up|down.

Markus Armbruster (4):
  monitor: Rename argument type 'b' to 'f'
  monitor: New argument type 'b'
  monitor: Use argument type 'b' for set_link
  monitor: Convert do_set_link() to QObject, QError

 monitor.c       |   39 +++++++++++++++++++++++++++++++++++----
 net.c           |   17 ++++++-----------
 net.h           |    2 +-
 qemu-monitor.hx |   13 +++++++------
 4 files changed, 49 insertions(+), 22 deletions(-)

[Qemu-devel] [PATCH 1/4] monitor: Rename argument type 'b' to 'f'

[Markus Armbruster]

To make 'b' available for boolean argument.

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

diff --git a/monitor.c b/monitor.c
index 35cbce7..3ce9a4e 100644
--- a/monitor.c
+++ b/monitor.c
@@ -74,7 +74,7 @@
  * '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
+ * '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
@@ -3798,7 +3798,7 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon,
                 qdict_put(qdict, key, qint_from_int(val));
             }
             break;
-        case 'b':
+        case 'f':
         case 'T':
             {
                 double val;
@@ -3814,7 +3814,7 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon,
                 if (get_double(mon, &val, &p) < 0) {
                     goto fail;
                 }
-                if (c == 'b' && *p) {
+                if (c == 'f' && *p) {
                     switch (*p) {
                     case 'K': case 'k':
                         val *= 1 << 10; p++; break;
@@ -4315,7 +4315,7 @@ static int check_arg(const CmdArgs *cmd_args, QDict *args)
                 return -1;
             }
             break;
-        case 'b':
+        case 'f':
         case 'T':
             if (qobject_type(value) != QTYPE_QINT && qobject_type(value) != QTYPE_QFLOAT) {
                 qerror_report(QERR_INVALID_PARAMETER_TYPE, name, "number");
diff --git a/qemu-monitor.hx b/qemu-monitor.hx
index 31087bd..8c9a41c 100644
--- a/qemu-monitor.hx
+++ b/qemu-monitor.hx
@@ -804,7 +804,7 @@ ETEXI
 
     {
         .name       = "migrate_set_speed",
-        .args_type  = "value:b",
+        .args_type  = "value:f",
         .params     = "value",
         .help       = "set maximum speed (in bytes) for migrations",
         .user_print = monitor_user_noop,
-- 
1.6.6.1

[Qemu-devel] [PATCH 2/4] monitor: New argument type 'b'

[Markus Armbruster]

This is a boolean value.  Human monitor accepts "on" or "off".
Consistent with option parsing (see parse_option_bool()).

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

diff --git a/monitor.c b/monitor.c
index 3ce9a4e..47b68a2 100644
--- a/monitor.c
+++ b/monitor.c
@@ -85,6 +85,8 @@
  *
  * '?'          optional type (for all types, except '/')
  * '.'          other form of optional type (for 'i' and 'l')
+ * 'b'          boolean
+ *              user mode accepts "on" or "off"
  * '-'          optional parameter (eg. '-f')
  *
  */
@@ -3841,6 +3843,29 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon,
                 qdict_put(qdict, key, qfloat_from_double(val));
             }
             break;
+        case 'b':
+            {
+                const char *beg;
+                int val;
+
+                while (qemu_isspace(*p)) {
+                    p++;
+                }
+                beg = p;
+                while (qemu_isgraph(*p)) {
+                    p++;
+                }
+                if (!strncmp(beg, "on", p - beg)) {
+                    val = 1;
+                } else if (!strncmp(beg, "off", p - beg)) {
+                    val = 0;
+                } else {
+                    monitor_printf(mon, "Expected 'on' or 'off'\n");
+                    goto fail;
+                }
+                qdict_put(qdict, key, qbool_from_int(val));
+            }
+            break;
         case '-':
             {
                 const char *tmp = p;
@@ -4322,6 +4347,12 @@ static int check_arg(const CmdArgs *cmd_args, QDict *args)
                 return -1;
             }
             break;
+        case 'b':
+            if (qobject_type(value) != QTYPE_QBOOL) {
+                qerror_report(QERR_INVALID_PARAMETER_TYPE, name, "bool");
+                return -1;
+            }
+            break;
         case '-':
             if (qobject_type(value) != QTYPE_QINT &&
                 qobject_type(value) != QTYPE_QBOOL) {
-- 
1.6.6.1

[Qemu-devel] Re: [PATCH 2/4] monitor: New argument type 'b'

[Luiz Capitulino]

On Tue, 23 Mar 2010 11:27:56 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> This is a boolean value.  Human monitor accepts "on" or "off".
> Consistent with option parsing (see parse_option_bool()).
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  monitor.c |   31 +++++++++++++++++++++++++++++++
>  1 files changed, 31 insertions(+), 0 deletions(-)
> 
> diff --git a/monitor.c b/monitor.c
> index 3ce9a4e..47b68a2 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -85,6 +85,8 @@
>   *
>   * '?'          optional type (for all types, except '/')
>   * '.'          other form of optional type (for 'i' and 'l')
> + * 'b'          boolean
> + *              user mode accepts "on" or "off"
>   * '-'          optional parameter (eg. '-f')
>   *
>   */
> @@ -3841,6 +3843,29 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon,
>                  qdict_put(qdict, key, qfloat_from_double(val));
>              }
>              break;
> +        case 'b':
> +            {
> +                const char *beg;
> +                int val;
> +
> +                while (qemu_isspace(*p)) {
> +                    p++;
> +                }
> +                beg = p;
> +                while (qemu_isgraph(*p)) {
> +                    p++;
> +                }
> +                if (!strncmp(beg, "on", p - beg)) {
> +                    val = 1;
> +                } else if (!strncmp(beg, "off", p - beg)) {
> +                    val = 0;
> +                } else {
> +                    monitor_printf(mon, "Expected 'on' or 'off'\n");
> +                    goto fail;
> +                }

 This will make 'on' be the default when no on/off is specified, is that
your intention? I'm wondering if this can cause problems when you add
optional support for it and mixes it with other arguments.

> +                qdict_put(qdict, key, qbool_from_int(val));
> +            }
> +            break;
>          case '-':
>              {
>                  const char *tmp = p;
> @@ -4322,6 +4347,12 @@ static int check_arg(const CmdArgs *cmd_args, QDict *args)
>                  return -1;
>              }
>              break;
> +        case 'b':
> +            if (qobject_type(value) != QTYPE_QBOOL) {
> +                qerror_report(QERR_INVALID_PARAMETER_TYPE, name, "bool");
> +                return -1;
> +            }
> +            break;
>          case '-':
>              if (qobject_type(value) != QTYPE_QINT &&
>                  qobject_type(value) != QTYPE_QBOOL) {

[Qemu-devel] Re: [PATCH 2/4] monitor: New argument type 'b'

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Tue, 23 Mar 2010 11:27:56 +0100
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> This is a boolean value.  Human monitor accepts "on" or "off".
>> Consistent with option parsing (see parse_option_bool()).
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  monitor.c |   31 +++++++++++++++++++++++++++++++
>>  1 files changed, 31 insertions(+), 0 deletions(-)
>> 
>> diff --git a/monitor.c b/monitor.c
>> index 3ce9a4e..47b68a2 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -85,6 +85,8 @@
>>   *
>>   * '?'          optional type (for all types, except '/')
>>   * '.'          other form of optional type (for 'i' and 'l')
>> + * 'b'          boolean
>> + *              user mode accepts "on" or "off"
>>   * '-'          optional parameter (eg. '-f')
>>   *
>>   */
>> @@ -3841,6 +3843,29 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon,
>>                  qdict_put(qdict, key, qfloat_from_double(val));
>>              }
>>              break;
>> +        case 'b':
>> +            {
>> +                const char *beg;
>> +                int val;
>> +
>> +                while (qemu_isspace(*p)) {
>> +                    p++;
>> +                }
>> +                beg = p;
>> +                while (qemu_isgraph(*p)) {
>> +                    p++;
>> +                }
>> +                if (!strncmp(beg, "on", p - beg)) {
>> +                    val = 1;
>> +                } else if (!strncmp(beg, "off", p - beg)) {
>> +                    val = 0;
>> +                } else {
>> +                    monitor_printf(mon, "Expected 'on' or 'off'\n");
>> +                    goto fail;
>> +                }
>
>  This will make 'on' be the default when no on/off is specified, is that
> your intention? I'm wondering if this can cause problems when you add
> optional support for it and mixes it with other arguments.

No.  Intended behavior: the argument must be either "on" or "off".  With
"on", (KEY, true) is put into the dictionary, for "off" it's (KEY,
false).

We get a third case for optional argument if we support that: KEY not in
dictionary.  The handler decides how to interpret that.

[...]

[Qemu-devel] Re: [PATCH 2/4] monitor: New argument type 'b'

[Luiz Capitulino]

On Thu, 25 Mar 2010 18:28:43 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > On Tue, 23 Mar 2010 11:27:56 +0100
> > Markus Armbruster <armbru@redhat.com> wrote:
> >
> >> This is a boolean value.  Human monitor accepts "on" or "off".
> >> Consistent with option parsing (see parse_option_bool()).
> >> 
> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> >> ---
> >>  monitor.c |   31 +++++++++++++++++++++++++++++++
> >>  1 files changed, 31 insertions(+), 0 deletions(-)
> >> 
> >> diff --git a/monitor.c b/monitor.c
> >> index 3ce9a4e..47b68a2 100644
> >> --- a/monitor.c
> >> +++ b/monitor.c
> >> @@ -85,6 +85,8 @@
> >>   *
> >>   * '?'          optional type (for all types, except '/')
> >>   * '.'          other form of optional type (for 'i' and 'l')
> >> + * 'b'          boolean
> >> + *              user mode accepts "on" or "off"
> >>   * '-'          optional parameter (eg. '-f')
> >>   *
> >>   */
> >> @@ -3841,6 +3843,29 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon,
> >>                  qdict_put(qdict, key, qfloat_from_double(val));
> >>              }
> >>              break;
> >> +        case 'b':
> >> +            {
> >> +                const char *beg;
> >> +                int val;
> >> +
> >> +                while (qemu_isspace(*p)) {
> >> +                    p++;
> >> +                }
> >> +                beg = p;
> >> +                while (qemu_isgraph(*p)) {
> >> +                    p++;
> >> +                }
> >> +                if (!strncmp(beg, "on", p - beg)) {
> >> +                    val = 1;
> >> +                } else if (!strncmp(beg, "off", p - beg)) {
> >> +                    val = 0;
> >> +                } else {
> >> +                    monitor_printf(mon, "Expected 'on' or 'off'\n");
> >> +                    goto fail;
> >> +                }
> >
> >  This will make 'on' be the default when no on/off is specified, is that
> > your intention? I'm wondering if this can cause problems when you add
> > optional support for it and mixes it with other arguments.
> 
> No.  Intended behavior: the argument must be either "on" or "off".  With
> "on", (KEY, true) is put into the dictionary, for "off" it's (KEY,
> false).
> 
> We get a third case for optional argument if we support that: KEY not in
> dictionary.  The handler decides how to interpret that.

 Ok, but strncmp() will return 0 if p - beg = 0, right? In this
case the current implementation will put true on the dict for a line like:

(qemu) set_link foo

 Which should return an error to the user then.

[Qemu-devel] Re: [PATCH 2/4] monitor: New argument type 'b'

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

>  Ok, but strncmp() will return 0 if p - beg = 0, right? In this
> case the current implementation will put true on the dict for a line like:
>
> (qemu) set_link foo
>
>  Which should return an error to the user then.

Brain fart.  I'll respin.  Thanks for catching this!

[Qemu-devel] [PATCH 3/4] monitor: Use argument type 'b' for set_link

[Markus Armbruster]

Second argument is now "on" or "off" instead of "up" or "down".

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 net.c           |   10 ++--------
 qemu-monitor.hx |    8 ++++----
 2 files changed, 6 insertions(+), 12 deletions(-)

diff --git a/net.c b/net.c
index 80e9025..673b6ec 100644
--- a/net.c
+++ b/net.c
@@ -1288,7 +1288,7 @@ void do_set_link(Monitor *mon, const QDict *qdict)
     VLANState *vlan;
     VLANClientState *vc = NULL;
     const char *name = qdict_get_str(qdict, "name");
-    const char *up_or_down = qdict_get_str(qdict, "up_or_down");
+    int up = qdict_get_bool(qdict, "up");
 
     QTAILQ_FOREACH(vlan, &vlans, next) {
         QTAILQ_FOREACH(vc, &vlan->clients, next) {
@@ -1305,13 +1305,7 @@ done:
         return;
     }
 
-    if (strcmp(up_or_down, "up") == 0)
-        vc->link_down = 0;
-    else if (strcmp(up_or_down, "down") == 0)
-        vc->link_down = 1;
-    else
-        monitor_printf(mon, "invalid link status '%s'; only 'up' or 'down' "
-                       "valid\n", up_or_down);
+    vc->link_down = !up;
 
     if (vc->info->link_status_changed) {
         vc->info->link_status_changed(vc);
diff --git a/qemu-monitor.hx b/qemu-monitor.hx
index 8c9a41c..7b7dcf5 100644
--- a/qemu-monitor.hx
+++ b/qemu-monitor.hx
@@ -986,16 +986,16 @@ ETEXI
 
     {
         .name       = "set_link",
-        .args_type  = "name:s,up_or_down:s",
-        .params     = "name up|down",
+        .args_type  = "name:s,up:b",
+        .params     = "name on|off",
         .help       = "change the link status of a network adapter",
         .mhandler.cmd = do_set_link,
     },
 
 STEXI
-@item set_link @var{name} [up|down]
+@item set_link @var{name} [on|off]
 @findex set_link
-Set link @var{name} up or down.
+Switch link @var{name} on (i.e. up) or off (i.e. down).
 ETEXI
 
     {
-- 
1.6.6.1

[Qemu-devel] [PATCH 4/4] monitor: Convert do_set_link() to QObject, QError

[Markus Armbruster]

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

diff --git a/net.c b/net.c
index 673b6ec..96f2670 100644
--- a/net.c
+++ b/net.c
@@ -1283,7 +1283,7 @@ void do_info_network(Monitor *mon)
     }
 }
 
-void do_set_link(Monitor *mon, const QDict *qdict)
+int do_set_link(Monitor *mon, const QDict *qdict, QObject **ret_data)
 {
     VLANState *vlan;
     VLANClientState *vc = NULL;
@@ -1301,8 +1301,8 @@ void do_set_link(Monitor *mon, const QDict *qdict)
 done:
 
     if (!vc) {
-        monitor_printf(mon, "could not find network device '%s'\n", name);
-        return;
+        qerror_report(QERR_DEVICE_NOT_FOUND, name);
+        return -1;
     }
 
     vc->link_down = !up;
@@ -1310,6 +1310,7 @@ done:
     if (vc->info->link_status_changed) {
         vc->info->link_status_changed(vc);
     }
+    return 0;
 }
 
 void net_cleanup(void)
diff --git a/net.h b/net.h
index ce9e2c6..c7a3a1b 100644
--- a/net.h
+++ b/net.h
@@ -118,7 +118,7 @@ int qemu_find_nic_model(NICInfo *nd, const char * const *models,
                         const char *default_model);
 
 void do_info_network(Monitor *mon);
-void do_set_link(Monitor *mon, const QDict *qdict);
+int do_set_link(Monitor *mon, const QDict *qdict, QObject **ret_data);
 
 /* NIC info */
 
diff --git a/qemu-monitor.hx b/qemu-monitor.hx
index 7b7dcf5..62fa346 100644
--- a/qemu-monitor.hx
+++ b/qemu-monitor.hx
@@ -989,7 +989,8 @@ ETEXI
         .args_type  = "name:s,up:b",
         .params     = "name on|off",
         .help       = "change the link status of a network adapter",
-        .mhandler.cmd = do_set_link,
+        .user_print = monitor_user_noop,
+        .mhandler.cmd_new = do_set_link,
     },
 
 STEXI
-- 
1.6.6.1

[Qemu-devel] Re: [PATCH 0/4] monitor: Convert do_set_link() to QObject, QError

[Luiz Capitulino]

On Tue, 23 Mar 2010 11:27:54 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> PATCH 3/4 changes syntax of set_link's second argument from up|down to
> on|off.  I feel that the argument needs to be boolean in QMP, and this
> is the simplest way to get it.
> 
> Alternatives I could try if the syntax change is unwanted:
> 
> * Use the old string argument in QMP.  Easy.
> 
> * Don't convert set_link, create a new command with a boolean
>   argument.
> 
> * Create a argument parser for up|down.

 I like your approach. Daniel do you use set_link in libvirt already?
I've grepped around I didn't found any reference for it.

> 
> Markus Armbruster (4):
>   monitor: Rename argument type 'b' to 'f'
>   monitor: New argument type 'b'
>   monitor: Use argument type 'b' for set_link
>   monitor: Convert do_set_link() to QObject, QError
> 
>  monitor.c       |   39 +++++++++++++++++++++++++++++++++++----
>  net.c           |   17 ++++++-----------
>  net.h           |    2 +-
>  qemu-monitor.hx |   13 +++++++------
>  4 files changed, 49 insertions(+), 22 deletions(-)
> 

[Qemu-devel] Re: [PATCH 0/4] monitor: Convert do_set_link() to QObject, QError

[Daniel P. Berrange]

On Wed, Mar 24, 2010 at 04:20:53PM -0300, Luiz Capitulino wrote:
> On Tue, 23 Mar 2010 11:27:54 +0100
> Markus Armbruster <armbru@redhat.com> wrote:
> 
> > PATCH 3/4 changes syntax of set_link's second argument from up|down to
> > on|off.  I feel that the argument needs to be boolean in QMP, and this
> > is the simplest way to get it.
> > 
> > Alternatives I could try if the syntax change is unwanted:
> > 
> > * Use the old string argument in QMP.  Easy.
> > 
> > * Don't convert set_link, create a new command with a boolean
> >   argument.
> > 
> > * Create a argument parser for up|down.
> 
>  I like your approach. Daniel do you use set_link in libvirt already?
> I've grepped around I didn't found any reference for it.

We don't currently use it, but plan to in the not too distant future.

Regards,
Daniel
-- 
|: Red Hat, Engineering, London    -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :|
|: http://autobuild.org        -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|