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

[Luiz Capitulino <lcapitulino@redhat.com>]

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;