| Message ID | 1264003702-17329-4-git-send-email-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
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;
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. >> + 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;
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;
Luiz Capitulino <lcapitulino@redhat.com> writes: > 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. We might want to ignore underflow silently, and fail loudly for ininity and NaN. Opinions? [...]
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; + 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; + } + *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; + } + qdict_put(qdict, key, qfloat_from_double(val)); + } + break; case '-': { const char *tmp = p;
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. Signed-off-by: Markus Armbruster <armbru@redhat.com> --- monitor.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 58 insertions(+), 0 deletions(-)