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

[Markus Armbruster]

Luiz Capitulino <address@hidden> writes:

> On Thu, 21 Jan 2010 15:04:43 +0100
> Markus Armbruster <address@hidden> wrote:
>
>> 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.
>
>  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?

[...]