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

[Markus Armbruster]

Luiz Capitulino <address@hidden> writes:

> On Wed, 20 Jan 2010 17:08:17 +0100
> Markus Armbruster <address@hidden> 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 <address@hidden>
>> ---
>>  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;