Patchwork [4/5] Switch migrate_set_speed() to take an 'o' argument rather than a float.

login
register
mail settings
Submitter Jes Sorensen
Date Sept. 16, 2010, 2:52 p.m.
Message ID <1284648749-18479-5-git-send-email-Jes.Sorensen@redhat.com>
Download mbox | patch
Permalink /patch/64990/
State New
Headers show

Comments

Jes Sorensen - Sept. 16, 2010, 2:52 p.m.
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(-)
Markus Armbruster - Sept. 28, 2010, 10:08 a.m.
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)?
Luiz Capitulino - Sept. 28, 2010, 2:32 p.m.
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)?
>
Jes Sorensen - Oct. 7, 2010, 2:12 p.m.
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

Patch

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,