| Message ID | 20181109110221.10553-3-david@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | qapi: rewrite string-input-visitor | expand |
David Hildenbrand <david@redhat.com> writes: > Let's use the new function. > > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > qapi/string-input-visitor.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c > index b3fdd0827d..dee708d384 100644 > --- a/qapi/string-input-visitor.c > +++ b/qapi/string-input-visitor.c > @@ -20,6 +20,7 @@ > #include "qemu/option.h" > #include "qemu/queue.h" > #include "qemu/range.h" > +#include "qemu/cutils.h" > > > struct StringInputVisitor > @@ -313,12 +314,9 @@ static void parse_type_number(Visitor *v, const char *name, double *obj, > Error **errp) > { > StringInputVisitor *siv = to_siv(v); > - char *endp = (char *) siv->string; > double val; > > - errno = 0; > - val = strtod(siv->string, &endp); > - if (errno || endp == siv->string || *endp) { > + if (qemu_strtod(siv->string, NULL, &val)) { > error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null", > "number"); > return; Three more: in qobject-input-visitor.c's qobject_input_type_number_keyval(), cutil.c's do_strtosz(), and json-parser.c's parse_literal(). The latter doesn't check for errors since the lexer ensures the input is sane. Overflow can still happen, and is silently ignored. Feel free not to convert this one.
On 14.11.18 17:09, Markus Armbruster wrote: > David Hildenbrand <david@redhat.com> writes: > >> Let's use the new function. >> >> Signed-off-by: David Hildenbrand <david@redhat.com> >> --- >> qapi/string-input-visitor.c | 6 ++---- >> 1 file changed, 2 insertions(+), 4 deletions(-) >> >> diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c >> index b3fdd0827d..dee708d384 100644 >> --- a/qapi/string-input-visitor.c >> +++ b/qapi/string-input-visitor.c >> @@ -20,6 +20,7 @@ >> #include "qemu/option.h" >> #include "qemu/queue.h" >> #include "qemu/range.h" >> +#include "qemu/cutils.h" >> >> >> struct StringInputVisitor >> @@ -313,12 +314,9 @@ static void parse_type_number(Visitor *v, const char *name, double *obj, >> Error **errp) >> { >> StringInputVisitor *siv = to_siv(v); >> - char *endp = (char *) siv->string; >> double val; >> >> - errno = 0; >> - val = strtod(siv->string, &endp); >> - if (errno || endp == siv->string || *endp) { >> + if (qemu_strtod(siv->string, NULL, &val)) { >> error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null", >> "number"); >> return; > > Three more: in qobject-input-visitor.c's > qobject_input_type_number_keyval(), This one is interesting, as it properly bails out when parsing "inf" (via isFinite()). - should we do the same for the string input visitor? Especially, should we forbid "inf" and "NaN" in both scenarios? cutil.c's do_strtosz(), and > json-parser.c's parse_literal(). > > The latter doesn't check for errors since the lexer ensures the input is > sane. Overflow can still happen, and is silently ignored. Feel free > not to convert this one. > I'll do the conversion of all (allowing -ERANGE where it used to be allow), we can then discuss with the patches at hand if it makes sense.
On 11/15/18 5:09 AM, David Hildenbrand wrote: >> Three more: in qobject-input-visitor.c's >> qobject_input_type_number_keyval(), > > This one is interesting, as it properly bails out when parsing "inf" > (via isFinite()). - should we do the same for the string input visitor? > > Especially, should we forbid "inf" and "NaN" in both scenarios? JSON can't represent non-finite doubles. Internally, we might be able to use them, but you have a point that consistently rejecting non-finite in all of our QAPI parsers makes it easier to reason about the code base (the command line can't be used to inject a value not possible via QMP). So that makes sense to me. qemu_strtod() shouldn't reject non-finite numbers (because it is useful for more than just qapi), but we could add a new qemu_strtod_finite() if that would help avoid duplication.
On 15.11.18 14:17, Eric Blake wrote: > On 11/15/18 5:09 AM, David Hildenbrand wrote: > >>> Three more: in qobject-input-visitor.c's >>> qobject_input_type_number_keyval(), >> >> This one is interesting, as it properly bails out when parsing "inf" >> (via isFinite()). - should we do the same for the string input visitor? >> >> Especially, should we forbid "inf" and "NaN" in both scenarios? > > JSON can't represent non-finite doubles. Internally, we might be able to > use them, but you have a point that consistently rejecting non-finite in > all of our QAPI parsers makes it easier to reason about the code base > (the command line can't be used to inject a value not possible via QMP). > So that makes sense to me. qemu_strtod() shouldn't reject non-finite > numbers (because it is useful for more than just qapi), but we could add > a new qemu_strtod_finite() if that would help avoid duplication. > Yes, I'll exactly add that! Thanks
Eric Blake <eblake@redhat.com> writes: > On 11/15/18 5:09 AM, David Hildenbrand wrote: > >>> Three more: in qobject-input-visitor.c's >>> qobject_input_type_number_keyval(), >> >> This one is interesting, as it properly bails out when parsing "inf" >> (via isFinite()). - should we do the same for the string input visitor? >> >> Especially, should we forbid "inf" and "NaN" in both scenarios? > > JSON can't represent non-finite doubles. Internally, we might be able > to use them, but you have a point that consistently rejecting > non-finite in all of our QAPI parsers makes it easier to reason about > the code base (the command line can't be used to inject a value not > possible via QMP). So that makes sense to me. Given the liberties we already take with JSON in QAPI, "JSON can't do X" need not immediately end a conversation about QAPI. That said, consistency between QObject and string visitor is desirable. The QObject input visitor comes in two flavours: one for use with the JSON parser, and one for use with keyval_parse(). The latter flavour's qobject_input_type_number_keyval() rejects non-finite numbers. The former flavour's qobject_input_type_number() leaves rejecting "bad" numbers to the JSON parser. The JSON parser doesn't accept special syntax for NaN or infinity. It maps large numbers to HUGE_VAL with the appropriate sign. Whether +/-HUGE_VAL are infinities depends on the host. If we really want to outlaw infinities, we better fix parse_literal() to check for ERANGE. > qemu_strtod() shouldn't > reject non-finite numbers (because it is useful for more than just > qapi), Yes. > but we could add a new qemu_strtod_finite() if that would help > avoid duplication. Maybe. qemu_strtod(str, NULL, &dbl) && isfinite(dbl) isn't that much shorter or clearer than qemu_strtod_finite(str, NULL, &dbl) >> cutil.c's do_strtosz(), and >> json-parser.c's parse_literal(). >> >> The latter doesn't check for errors since the lexer ensures the input is >> sane. Overflow can still happen, and is silently ignored. Feel free >> not to convert this one. >> > > I'll do the conversion of all (allowing -ERANGE where it used to be > allow), we can then discuss with the patches at hand if it makes sense. Sure!
diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c index b3fdd0827d..dee708d384 100644 --- a/qapi/string-input-visitor.c +++ b/qapi/string-input-visitor.c @@ -20,6 +20,7 @@ #include "qemu/option.h" #include "qemu/queue.h" #include "qemu/range.h" +#include "qemu/cutils.h" struct StringInputVisitor @@ -313,12 +314,9 @@ static void parse_type_number(Visitor *v, const char *name, double *obj, Error **errp) { StringInputVisitor *siv = to_siv(v); - char *endp = (char *) siv->string; double val; - errno = 0; - val = strtod(siv->string, &endp); - if (errno || endp == siv->string || *endp) { + if (qemu_strtod(siv->string, NULL, &val)) { error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null", "number"); return;
Let's use the new function. Signed-off-by: David Hildenbrand <david@redhat.com> --- qapi/string-input-visitor.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)