Patchwork [v2,3/8] monitor: New argument type 'b'

login
register
mail settings
Submitter Markus Armbruster
Date Jan. 20, 2010, 4:08 p.m.
Message ID <1264003702-17329-4-git-send-email-armbru@redhat.com>
Download mbox | patch
Permalink /patch/43303/
State New
Headers show

Comments

Markus Armbruster - Jan. 20, 2010, 4:08 p.m.
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(-)
Luiz Capitulino - Jan. 21, 2010, 1:17 p.m.
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;
Markus Armbruster - Jan. 21, 2010, 2:04 p.m.
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;
Luiz Capitulino - Jan. 21, 2010, 2:28 p.m.
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;
Markus Armbruster - Jan. 21, 2010, 2:59 p.m.
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?

[...]

Patch

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;