On Thu, 21 Jan 2010 15:04:43 +0100
Markus Armbruster <arm...@redhat.com> wrote:
> Luiz Capitulino <lcapitul...@redhat.com> writes:
>
> > On Wed, 20 Jan 2010 17:08:17 +0100
> > Markus Armbruster <arm...@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 <arm...@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;
