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

[Luiz Capitulino <lcapitulino@redhat.com>]

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;