| Message ID | 1473088600-17930-6-git-send-email-berrange@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
"Daniel P. Berrange" <berrange@redhat.com> writes: > Currently the QmpInputVisitor assumes that all scalar > values are directly represented as their final types. > ie it assumes an 'int' is using QInt, and a 'bool' is > using QBool. > > This adds an alternative constructor for QmpInputVisitor > that will set it up such that it expects a QString for > all scalar types instead. > > This makes it possible to use QmpInputVisitor with a > QDict produced from QemuOpts, where everything is in > string format. > > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> > Signed-off-by: Daniel P. Berrange <berrange@redhat.com> Ignorant question: is this used by the next patch?
On Mon, Sep 12, 2016 at 06:21:32PM +0200, Markus Armbruster wrote: > "Daniel P. Berrange" <berrange@redhat.com> writes: > > > Currently the QmpInputVisitor assumes that all scalar > > values are directly represented as their final types. > > ie it assumes an 'int' is using QInt, and a 'bool' is > > using QBool. > > > > This adds an alternative constructor for QmpInputVisitor > > that will set it up such that it expects a QString for > > all scalar types instead. > > > > This makes it possible to use QmpInputVisitor with a > > QDict produced from QemuOpts, where everything is in > > string format. > > > > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > Signed-off-by: Daniel P. Berrange <berrange@redhat.com> > > Ignorant question: is this used by the next patch? Yes indeed :-) We use the qdict_crumpl() method to turn the command line QemuOpts into a QDict, and then this new mode for QmpInputVisitor for the visiting this QDict, thus eliminating need to use OptsVisitor in QOM. Regards, Daniel
On Mon, Sep 12, 2016 at 05:23:47PM +0100, Daniel P. Berrange wrote: > On Mon, Sep 12, 2016 at 06:21:32PM +0200, Markus Armbruster wrote: > > "Daniel P. Berrange" <berrange@redhat.com> writes: > > > > > Currently the QmpInputVisitor assumes that all scalar > > > values are directly represented as their final types. > > > ie it assumes an 'int' is using QInt, and a 'bool' is > > > using QBool. > > > > > > This adds an alternative constructor for QmpInputVisitor > > > that will set it up such that it expects a QString for > > > all scalar types instead. > > > > > > This makes it possible to use QmpInputVisitor with a > > > QDict produced from QemuOpts, where everything is in > > > string format. > > > > > > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > > Signed-off-by: Daniel P. Berrange <berrange@redhat.com> > > > > Ignorant question: is this used by the next patch? > > Yes indeed :-) > > We use the qdict_crumpl() method to turn the command line QemuOpts > into a QDict, and then this new mode for QmpInputVisitor for the > visiting this QDict, thus eliminating need to use OptsVisitor in > QOM. Oh and obviously my commit message needs fixing to say QObjectInputVisitor not QmpInputVisitor now :-) Regards, Daniel
On 09/05/2016 10:16 AM, Daniel P. Berrange wrote: > Currently the QmpInputVisitor assumes that all scalar > values are directly represented as their final types. > ie it assumes an 'int' is using QInt, and a 'bool' is > using QBool. > > This adds an alternative constructor for QmpInputVisitor > that will set it up such that it expects a QString for > all scalar types instead. > > This makes it possible to use QmpInputVisitor with a > QDict produced from QemuOpts, where everything is in > string format. > > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> > Signed-off-by: Daniel P. Berrange <berrange@redhat.com> > --- > include/qapi/qobject-input-visitor.h | 41 +++++++++- > qapi/qobject-input-visitor.c | 115 +++++++++++++++++++++++++- > tests/test-qobject-input-visitor.c | 152 ++++++++++++++++++++++++++++++++++- > 3 files changed, 298 insertions(+), 10 deletions(-) > > +/** > + * qobject_string_input_visitor_new: > + * @obj: the input object to visit > + * > + * Create a new input visitor that converts a QObject to a QAPI object. > + * > + * Any scalar values in the @obj input data structure should always be > + * represented as strings. i.e. if visiting a boolean, the value should > + * be a QString whose contents represent a valid boolean. > + * > + * The visitor always operates in strict mode, requiring all dict keys > + * to be consumed during visitation. Good; I like that strict mode on the new constructor is not optional. > +static void qobject_input_type_int64_str(Visitor *v, const char *name, > + int64_t *obj, Error **errp) > +{ > + QObjectInputVisitor *qiv = to_qiv(v); > + QString *qstr = qobject_to_qstring(qobject_input_get_object(qiv, name, > + true)); > + uint64_t ret; Uninitialized... > + > + parse_option_number(name, qstr ? qstr->string : NULL, &ret, errp); ...and parse_option_number() explicitly leaves *ret untouched on error... > + *obj = ret; so if errp was set, then *obj now contains uninitialized memory. I guess valgrind is smart enough to only complain if callers then try to branch based on that value (that is, assigning one uninit location to another silently propagates uninit status to the new location, but it is only when you branch or otherwise USE uninit data, not just copy it, that valgrind complains). On the other hand, if the caller explicitly set value = 0 before calling qobject_input_type_int64_str(&value), we've now messed with the caller's expectations of value being at its pre-set value on error. > @@ -312,8 +345,8 @@ static void qobject_input_type_str(Visitor *v, const char *name, char **obj, > } > > static void qobject_input_type_number(Visitor *v, const char *name, double *obj, > - Error **errp) > -{ > + Error **errp){ > + Spurious hunk. > +static void qobject_input_type_size_str(Visitor *v, const char *name, > + uint64_t *obj, Error **errp) > +{ > + QObjectInputVisitor *qiv = to_qiv(v); > + QString *qstr = qobject_to_qstring(qobject_input_get_object(qiv, name, > + true)); > + int64_t val; > + char *endptr; > + > + if (qstr && qstr->string) { > + val = qemu_strtosz_suffix(qstr->string, &endptr, > + QEMU_STRTOSZ_DEFSUFFIX_B); > + if (val < 0 || *endptr) { > + error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name, > + "a size value representible as a non-negative int64"); s/representible/representable/ > +++ b/tests/test-qobject-input-visitor.c > > +static void test_visitor_in_int_autocast(TestInputVisitorData *data, > + const void *unused) > +{ > + int64_t res = 0, value = -42; > + Visitor *v; > + > + v = visitor_input_test_init_full(data, true, true, > + "\"-42\""); > + > + visit_type_int(v, NULL, &res, &error_abort); > + g_assert_cmpint(res, ==, value); > +} > + > +static void test_visitor_in_int_noautocast(TestInputVisitorData *data, > + const void *unused) > +{ > + int64_t res = 0; > + Visitor *v; > + Error *err = NULL; > + > + v = visitor_input_test_init(data, "\"-42\""); > + > + visit_type_int(v, NULL, &res, &err); > + g_assert(err != NULL); > + error_free(err); > +} So we've previously tested that int->int works without autocast, and you add that str->int works with autocast, and that str->int fails without autocast. Is it also worth testing that int->int fails with autocast (that is, when doing string parsing, a QInt is intentionally rejected even though we are parsing to get an int result)? > @@ -841,10 +969,26 @@ int main(int argc, char **argv) > &in_visitor_data, test_visitor_in_int); > input_visitor_test_add("/visitor/input/int_overflow", > &in_visitor_data, test_visitor_in_int_overflow); > + input_visitor_test_add("/visitor/input/int_autocast", > + &in_visitor_data, test_visitor_in_int_autocast); > + input_visitor_test_add("/visitor/input/int_noautocast", > + &in_visitor_data, test_visitor_in_int_noautocast); > input_visitor_test_add("/visitor/input/bool", > &in_visitor_data, test_visitor_in_bool); > + input_visitor_test_add("/visitor/input/bool_autocast", > + &in_visitor_data, test_visitor_in_bool_autocast); > + input_visitor_test_add("/visitor/input/bool_noautocast", > + &in_visitor_data, test_visitor_in_bool_noautocast); > input_visitor_test_add("/visitor/input/number", > &in_visitor_data, test_visitor_in_number); > + input_visitor_test_add("/visitor/input/number_autocast", > + &in_visitor_data, test_visitor_in_number_autocast); > + input_visitor_test_add("/visitor/input/number_noautocast", > + &in_visitor_data, test_visitor_in_number_noautocast); > + input_visitor_test_add("/visitor/input/size_autocast", > + &in_visitor_data, test_visitor_in_size_autocast); > + input_visitor_test_add("/visitor/input/size_noautocast", > + &in_visitor_data, test_visitor_in_size_noautocast); Similar question for autocast causing QBool->bool, QInt->int under size, and QFloat->number failures.
"Daniel P. Berrange" <berrange@redhat.com> writes: > Currently the QmpInputVisitor assumes that all scalar > values are directly represented as their final types. > ie it assumes an 'int' is using QInt, and a 'bool' is > using QBool. > > This adds an alternative constructor for QmpInputVisitor > that will set it up such that it expects a QString for > all scalar types instead. > > This makes it possible to use QmpInputVisitor with a > QDict produced from QemuOpts, where everything is in > string format. Can you explain how this is related to the Options visitor? > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> > Signed-off-by: Daniel P. Berrange <berrange@redhat.com> > --- > include/qapi/qobject-input-visitor.h | 41 +++++++++- > qapi/qobject-input-visitor.c | 115 +++++++++++++++++++++++++- > tests/test-qobject-input-visitor.c | 152 ++++++++++++++++++++++++++++++++++- > 3 files changed, 298 insertions(+), 10 deletions(-) > > diff --git a/include/qapi/qobject-input-visitor.h b/include/qapi/qobject-input-visitor.h > index cde328d..aa911cb 100644 > --- a/include/qapi/qobject-input-visitor.h > +++ b/include/qapi/qobject-input-visitor.h > @@ -19,12 +19,45 @@ > > typedef struct QObjectInputVisitor QObjectInputVisitor; > > -/* > - * Return a new input visitor that converts a QObject to a QAPI object. > +/** > + * qobject_input_visitor_new: > + * @obj: the input object to visit > + * @strict: whether to require that all input keys are consumed > + * > + * Create a new input visitor that converts a QObject to a QAPI object. > + * > + * Any scalar values in the @obj input data structure should be in the > + * required type already. i.e. if visiting a bool, the value should > + * already be a QBool instance. > * > - * Set @strict to reject a parse that doesn't consume all keys of a > - * dictionary; otherwise excess input is ignored. > + * If @strict is set to true, then an error will be reported if any > + * dict keys are not consumed during visitation. > + * > + * The returned input visitor should be released by calling > + * visit_free() when no longer required. > + * > + * Returns: a new input visitor > */ GDK-Doc style is an egregious waste of screen space and reader bandwidth. Much of the text is spent on restating the obvious, drowning out the parts that are actually interesting. Compare: /* - * Return a new input visitor that converts a QObject to a QAPI object. + * Create an input visitor that converts @obj to a QAPI object. * - * Set @strict to reject a parse that doesn't consume all keys of a - * dictionary; otherwise excess input is ignored. + * + * If @strict is true, the visit is expected to consume all of @obj's + * keys. Else, excess keys are silently ignored. + * + * Return the input visitor. It should be destroyed with visit_free(). + */ Like your version, this is vague on what exactly happens when a strict visit fails to consume all keys. It's more explicitly vague, though :) GDK-Doc is that way so that a fairly dumb tool can generate fairly usable library reference documentation. We're not using that tool. This is not a library. I loathe having my read bandwidth wasted. > Visitor *qobject_input_visitor_new(QObject *obj, bool strict); > > +/** > + * qobject_string_input_visitor_new: > + * @obj: the input object to visit > + * > + * Create a new input visitor that converts a QObject to a QAPI object. > + * > + * Any scalar values in the @obj input data structure should always be > + * represented as strings. i.e. if visiting a boolean, the value should > + * be a QString whose contents represent a valid boolean. > + * > + * The visitor always operates in strict mode, requiring all dict keys > + * to be consumed during visitation. > + * > + * The returned input visitor should be released by calling > + * visit_free() when no longer required. > + * > + * Returns: a new input visitor > + */ > +Visitor *qobject_string_input_visitor_new(QObject *obj); > + > #endif [...]
On Tue, Sep 13, 2016 at 11:05:08AM +0200, Markus Armbruster wrote: > "Daniel P. Berrange" <berrange@redhat.com> writes: > > > Currently the QmpInputVisitor assumes that all scalar > > values are directly represented as their final types. > > ie it assumes an 'int' is using QInt, and a 'bool' is > > using QBool. > > > > This adds an alternative constructor for QmpInputVisitor > > that will set it up such that it expects a QString for > > all scalar types instead. > > > > This makes it possible to use QmpInputVisitor with a > > QDict produced from QemuOpts, where everything is in > > string format. > > Can you explain how this is related to the Options visitor? The intention is that this can replace the existing OptsVisitor, for cases that don't rely on the magic "List of scalars" semantics of OptsVisitor - eg where 'foo=3,foo=5,foo=533' gets turned into a QList. When using QemuOpts w/ qdict_crumple + QmpInputVisitor, you would do list of scalars in different manner 'foo.1=3,foo.2=5,foo.3=533' since this syntax is extendable to deal with arbitrary nesting of dicts + lists, where as the OptsVisitor syntax cannot be extended in a back-compatible manner. This series converts -object to use QmpInputVisitor and this is safe todo right now, since no currently created QOM object has a property which is a list of scalars and this the changed syntax is not going to affect any existing usage. The -drive arg can be converted to QmpInputVisitor too, since the qdict_crumple/QmpInputVisitor combination was explicitly designed to be 100% compatible with -drive syntax for blockdevs nested options. Other args would have to be considered on a case-by-case basis to see if they rely on the magic list of scalars syntax used by OptsVisitor. Probably only a few of them need this. > > --- > > include/qapi/qobject-input-visitor.h | 41 +++++++++- > > qapi/qobject-input-visitor.c | 115 +++++++++++++++++++++++++- > > tests/test-qobject-input-visitor.c | 152 ++++++++++++++++++++++++++++++++++- > > 3 files changed, 298 insertions(+), 10 deletions(-) Regards, Daniel
On Mon, Sep 12, 2016 at 01:39:50PM -0500, Eric Blake wrote: > On 09/05/2016 10:16 AM, Daniel P. Berrange wrote: > > Currently the QmpInputVisitor assumes that all scalar > > values are directly represented as their final types. > > ie it assumes an 'int' is using QInt, and a 'bool' is > > using QBool. > > > > This adds an alternative constructor for QmpInputVisitor > > that will set it up such that it expects a QString for > > all scalar types instead. > > > > This makes it possible to use QmpInputVisitor with a > > QDict produced from QemuOpts, where everything is in > > string format. > > > > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > Signed-off-by: Daniel P. Berrange <berrange@redhat.com> > > --- > > include/qapi/qobject-input-visitor.h | 41 +++++++++- > > qapi/qobject-input-visitor.c | 115 +++++++++++++++++++++++++- > > tests/test-qobject-input-visitor.c | 152 ++++++++++++++++++++++++++++++++++- > > 3 files changed, 298 insertions(+), 10 deletions(-) > > > > > +/** > > + * qobject_string_input_visitor_new: > > + * @obj: the input object to visit > > + * > > + * Create a new input visitor that converts a QObject to a QAPI object. > > + * > > + * Any scalar values in the @obj input data structure should always be > > + * represented as strings. i.e. if visiting a boolean, the value should > > + * be a QString whose contents represent a valid boolean. > > + * > > + * The visitor always operates in strict mode, requiring all dict keys > > + * to be consumed during visitation. > > Good; I like that strict mode on the new constructor is not optional. > > > > +static void qobject_input_type_int64_str(Visitor *v, const char *name, > > + int64_t *obj, Error **errp) > > +{ > > + QObjectInputVisitor *qiv = to_qiv(v); > > + QString *qstr = qobject_to_qstring(qobject_input_get_object(qiv, name, > > + true)); > > + uint64_t ret; > > Uninitialized... > > > + > > + parse_option_number(name, qstr ? qstr->string : NULL, &ret, errp); > > ...and parse_option_number() explicitly leaves *ret untouched on error... > > > + *obj = ret; > > so if errp was set, then *obj now contains uninitialized memory. I > guess valgrind is smart enough to only complain if callers then try to > branch based on that value (that is, assigning one uninit location to > another silently propagates uninit status to the new location, but it is > only when you branch or otherwise USE uninit data, not just copy it, > that valgrind complains). On the other hand, if the caller explicitly > set value = 0 before calling qobject_input_type_int64_str(&value), we've > now messed with the caller's expectations of value being at its pre-set > value on error. I'll use a local Error, so we can avoid the uninitialized value and also avoid splattering *obj on failure. > > +++ b/tests/test-qobject-input-visitor.c > > > > +static void test_visitor_in_int_autocast(TestInputVisitorData *data, > > + const void *unused) > > +{ > > + int64_t res = 0, value = -42; > > + Visitor *v; > > + > > + v = visitor_input_test_init_full(data, true, true, > > + "\"-42\""); > > + > > + visit_type_int(v, NULL, &res, &error_abort); > > + g_assert_cmpint(res, ==, value); > > +} > > + > > +static void test_visitor_in_int_noautocast(TestInputVisitorData *data, > > + const void *unused) > > +{ > > + int64_t res = 0; > > + Visitor *v; > > + Error *err = NULL; > > + > > + v = visitor_input_test_init(data, "\"-42\""); > > + > > + visit_type_int(v, NULL, &res, &err); > > + g_assert(err != NULL); > > + error_free(err); > > +} > > So we've previously tested that int->int works without autocast, and you > add that str->int works with autocast, and that str->int fails without > autocast. Is it also worth testing that int->int fails with autocast > (that is, when doing string parsing, a QInt is intentionally rejected > even though we are parsing to get an int result)? [snip] > Similar question for autocast causing QBool->bool, QInt->int under size, > and QFloat->number failures. You always notice all the edge cases :-) I've added such tests and it exposed a bug in my impl which is nice :-) Regards, Daniel
"Daniel P. Berrange" <berrange@redhat.com> writes: > On Tue, Sep 13, 2016 at 11:05:08AM +0200, Markus Armbruster wrote: >> "Daniel P. Berrange" <berrange@redhat.com> writes: >> >> > Currently the QmpInputVisitor assumes that all scalar >> > values are directly represented as their final types. >> > ie it assumes an 'int' is using QInt, and a 'bool' is >> > using QBool. >> > >> > This adds an alternative constructor for QmpInputVisitor >> > that will set it up such that it expects a QString for >> > all scalar types instead. >> > >> > This makes it possible to use QmpInputVisitor with a >> > QDict produced from QemuOpts, where everything is in >> > string format. >> >> Can you explain how this is related to the Options visitor? > > The intention is that this can replace the existing OptsVisitor, > for cases that don't rely on the magic "List of scalars" semantics > of OptsVisitor - eg where 'foo=3,foo=5,foo=533' gets turned into > a QList. There's also the even more magical foo=3-5,foo=533. > When using QemuOpts w/ qdict_crumple + QmpInputVisitor, you would > do list of scalars in different manner 'foo.1=3,foo.2=5,foo.3=533' > since this syntax is extendable to deal with arbitrary nesting of > dicts + lists, where as the OptsVisitor syntax cannot be extended > in a back-compatible manner. > > This series converts -object to use QmpInputVisitor and this is > safe todo right now, since no currently created QOM object has > a property which is a list of scalars and this the changed syntax > is not going to affect any existing usage. Peeking at the patch... aha! Instead of having the options visitor visit the QemuOpts, you convert the QemuOpts to a QDict, which you then visit with your new visitor. Less efficient, because you have to build and destroy the intermediate QDict. Not an issue when processing configuration or even monitor commands, of course. I guess you convert to QDict so that the work of going from a QemuOpts-style string using -drive conventions to a visit splits into manageable chunks more easily, preferably into chunks that already exist: parse string into QemuOpts, convert to QDict, crumple, visit, destroy QDict. Still, I take the conversion as a signal that our data structures are wrong. Wild idea: should QemuOpts use a QDict rather than a QTAILQ of QemuOpt to store options? The pipeline then becomes parse string into QemuOpts, clone its QDict, crumple, visit, destroy QDict. Next step would be crumpling in place, i.e. parse string into nested QemuOpts, get its QDict, visit. Mind, I'm not demanding you to do that now, I'm just trying to figure out whether this series is shoveling into or out of the QemuOpts hole :) > The -drive arg can be converted to QmpInputVisitor too, since > the qdict_crumple/QmpInputVisitor combination was explicitly > designed to be 100% compatible with -drive syntax for blockdevs > nested options. You abstaining from inventing yet another option syntax dialect is appreciated. > Other args would have to be considered on a case-by-case basis > to see if they rely on the magic list of scalars syntax used > by OptsVisitor. Probably only a few of them need this. Obvious maintainer question: what would it take to get rid of the options visitor entirely? Because this maintainer looks on additions to his fiefdom much more kindly when they're balanced by subtractions. I guess all the uses that don't rely on the "list of scalars" magic are easy prey: replace by conversion to intermediate QDict + your new visitor. What about the ones that do rely on it? Which ones do? Any ideas on how to change them so that they don't require a complete options visitor?
On Tue, Sep 13, 2016 at 03:32:33PM +0200, Markus Armbruster wrote: > "Daniel P. Berrange" <berrange@redhat.com> writes: > > When using QemuOpts w/ qdict_crumple + QmpInputVisitor, you would > > do list of scalars in different manner 'foo.1=3,foo.2=5,foo.3=533' > > since this syntax is extendable to deal with arbitrary nesting of > > dicts + lists, where as the OptsVisitor syntax cannot be extended > > in a back-compatible manner. > > > > This series converts -object to use QmpInputVisitor and this is > > safe todo right now, since no currently created QOM object has > > a property which is a list of scalars and this the changed syntax > > is not going to affect any existing usage. > > Peeking at the patch... aha! Instead of having the options visitor > visit the QemuOpts, you convert the QemuOpts to a QDict, which you then > visit with your new visitor. Less efficient, because you have to build > and destroy the intermediate QDict. Not an issue when processing > configuration or even monitor commands, of course. > > I guess you convert to QDict so that the work of going from a > QemuOpts-style string using -drive conventions to a visit splits into > manageable chunks more easily, preferably into chunks that already > exist: parse string into QemuOpts, convert to QDict, crumple, visit, > destroy QDict. FWIW, I did originally try to modify the QemuOpts visitor directly to support visiting nested data structure, but I basically ended up re-inventing alot of code from qdict and indeed having to use a qdict internally to QemuOpts to store intermediate bits. At that point I realized it was clearly silly to try to shoe horn it into QemuOpts visitor directly, since I was basically creating qdict_crumple() indirectly and then essentially making QemuOpts visitor have the same logic as QmpInputVisitor. > Still, I take the conversion as a signal that our data structures are > wrong. Wild idea: should QemuOpts use a QDict rather than a QTAILQ of > QemuOpt to store options? If QemuOpts used a QDict internally, then you avoid the first qemu_opts_to_qdict() call before qdict_crumple(), but everything else basically stays the same. So a minor improvement, but nothing ground-shaking. > The pipeline then becomes parse string into QemuOpts, clone its QDict, > crumple, visit, destroy QDict. Next step would be crumpling in place, > i.e. parse string into nested QemuOpts, get its QDict, visit. Mind, I'm > not demanding you to do that now, I'm just trying to figure out whether > this series is shoveling into or out of the QemuOpts hole :) Yes, I guess the interesting win would be if qemu_opts_do_parse() could directly populate a qdict in crumpled format from the start, thus avoiding the late qdict_crumple call altogether. > > Other args would have to be considered on a case-by-case basis > > to see if they rely on the magic list of scalars syntax used > > by OptsVisitor. Probably only a few of them need this. > > Obvious maintainer question: what would it take to get rid of the > options visitor entirely? Because this maintainer looks on additions to > his fiefdom much more kindly when they're balanced by subtractions. > > I guess all the uses that don't rely on the "list of scalars" magic are > easy prey: replace by conversion to intermediate QDict + your new > visitor. > > What about the ones that do rely on it? Which ones do? Any ideas on > how to change them so that they don't require a complete options > visitor? I guess you could have some code that re-processes the stored QemuOpt in-place, changing repeated keys such as 'foo=400,foo=342' into the preferred format 'foo.1=400,foo.2=342', at which point you can use the qdict_crumple facility directly. That's probably not even very hard. Regards, Daniel
Am 05.09.2016 um 17:16 hat Daniel P. Berrange geschrieben: > Currently the QmpInputVisitor assumes that all scalar > values are directly represented as their final types. > ie it assumes an 'int' is using QInt, and a 'bool' is > using QBool. > > This adds an alternative constructor for QmpInputVisitor > that will set it up such that it expects a QString for > all scalar types instead. > > This makes it possible to use QmpInputVisitor with a > QDict produced from QemuOpts, where everything is in > string format. > > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> > Signed-off-by: Daniel P. Berrange <berrange@redhat.com> > +static void qobject_input_type_size_str(Visitor *v, const char *name, > + uint64_t *obj, Error **errp) > +{ > + QObjectInputVisitor *qiv = to_qiv(v); > + QString *qstr = qobject_to_qstring(qobject_input_get_object(qiv, name, > + true)); > + int64_t val; > + char *endptr; > + > + if (qstr && qstr->string) { > + val = qemu_strtosz_suffix(qstr->string, &endptr, > + QEMU_STRTOSZ_DEFSUFFIX_B); > + if (val < 0 || *endptr) { > + error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name, > + "a size value representible as a non-negative int64"); > + return; > + } > + > + *obj = val; > + return; > + } > + > + error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null", > + "size"); > +} Why not use parse_option_size() here when you use the QemuOpts parser for the other functions? Of course, parse_option_size() could be switched to internally use qemu_strtosz_suffix() sooner or later, too, but that's a different problem... Kevin
On Wed, Sep 14, 2016 at 04:59:50PM +0200, Kevin Wolf wrote: > Am 05.09.2016 um 17:16 hat Daniel P. Berrange geschrieben: > > Currently the QmpInputVisitor assumes that all scalar > > values are directly represented as their final types. > > ie it assumes an 'int' is using QInt, and a 'bool' is > > using QBool. > > > > This adds an alternative constructor for QmpInputVisitor > > that will set it up such that it expects a QString for > > all scalar types instead. > > > > This makes it possible to use QmpInputVisitor with a > > QDict produced from QemuOpts, where everything is in > > string format. > > > > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > Signed-off-by: Daniel P. Berrange <berrange@redhat.com> > > > +static void qobject_input_type_size_str(Visitor *v, const char *name, > > + uint64_t *obj, Error **errp) > > +{ > > + QObjectInputVisitor *qiv = to_qiv(v); > > + QString *qstr = qobject_to_qstring(qobject_input_get_object(qiv, name, > > + true)); > > + int64_t val; > > + char *endptr; > > + > > + if (qstr && qstr->string) { > > + val = qemu_strtosz_suffix(qstr->string, &endptr, > > + QEMU_STRTOSZ_DEFSUFFIX_B); > > + if (val < 0 || *endptr) { > > + error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name, > > + "a size value representible as a non-negative int64"); > > + return; > > + } > > + > > + *obj = val; > > + return; > > + } > > + > > + error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null", > > + "size"); > > +} > > Why not use parse_option_size() here when you use the QemuOpts parser > for the other functions? No idea why i didn't use that. > Of course, parse_option_size() could be switched to internally use > qemu_strtosz_suffix() sooner or later, too, but that's a different > problem... Yes, it really should. I might just do that later. Regards, Daniel
diff --git a/include/qapi/qobject-input-visitor.h b/include/qapi/qobject-input-visitor.h index cde328d..aa911cb 100644 --- a/include/qapi/qobject-input-visitor.h +++ b/include/qapi/qobject-input-visitor.h @@ -19,12 +19,45 @@ typedef struct QObjectInputVisitor QObjectInputVisitor; -/* - * Return a new input visitor that converts a QObject to a QAPI object. +/** + * qobject_input_visitor_new: + * @obj: the input object to visit + * @strict: whether to require that all input keys are consumed + * + * Create a new input visitor that converts a QObject to a QAPI object. + * + * Any scalar values in the @obj input data structure should be in the + * required type already. i.e. if visiting a bool, the value should + * already be a QBool instance. * - * Set @strict to reject a parse that doesn't consume all keys of a - * dictionary; otherwise excess input is ignored. + * If @strict is set to true, then an error will be reported if any + * dict keys are not consumed during visitation. + * + * The returned input visitor should be released by calling + * visit_free() when no longer required. + * + * Returns: a new input visitor */ Visitor *qobject_input_visitor_new(QObject *obj, bool strict); +/** + * qobject_string_input_visitor_new: + * @obj: the input object to visit + * + * Create a new input visitor that converts a QObject to a QAPI object. + * + * Any scalar values in the @obj input data structure should always be + * represented as strings. i.e. if visiting a boolean, the value should + * be a QString whose contents represent a valid boolean. + * + * The visitor always operates in strict mode, requiring all dict keys + * to be consumed during visitation. + * + * The returned input visitor should be released by calling + * visit_free() when no longer required. + * + * Returns: a new input visitor + */ +Visitor *qobject_string_input_visitor_new(QObject *obj); + #endif diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c index 5ff3db3..b79c229 100644 --- a/qapi/qobject-input-visitor.c +++ b/qapi/qobject-input-visitor.c @@ -20,6 +20,7 @@ #include "qemu-common.h" #include "qapi/qmp/types.h" #include "qapi/qmp/qerror.h" +#include "qemu/cutils.h" #define QIV_STACK_SIZE 1024 @@ -263,6 +264,18 @@ static void qobject_input_type_int64(Visitor *v, const char *name, int64_t *obj, *obj = qint_get_int(qint); } +static void qobject_input_type_int64_str(Visitor *v, const char *name, + int64_t *obj, Error **errp) +{ + QObjectInputVisitor *qiv = to_qiv(v); + QString *qstr = qobject_to_qstring(qobject_input_get_object(qiv, name, + true)); + uint64_t ret; + + parse_option_number(name, qstr ? qstr->string : NULL, &ret, errp); + *obj = ret; +} + static void qobject_input_type_uint64(Visitor *v, const char *name, uint64_t *obj, Error **errp) { @@ -279,6 +292,16 @@ static void qobject_input_type_uint64(Visitor *v, const char *name, *obj = qint_get_int(qint); } +static void qobject_input_type_uint64_str(Visitor *v, const char *name, + uint64_t *obj, Error **errp) +{ + QObjectInputVisitor *qiv = to_qiv(v); + QString *qstr = qobject_to_qstring(qobject_input_get_object(qiv, name, + true)); + + parse_option_number(name, qstr ? qstr->string : NULL, obj, errp); +} + static void qobject_input_type_bool(Visitor *v, const char *name, bool *obj, Error **errp) { @@ -294,6 +317,16 @@ static void qobject_input_type_bool(Visitor *v, const char *name, bool *obj, *obj = qbool_get_bool(qbool); } +static void qobject_input_type_bool_str(Visitor *v, const char *name, bool *obj, + Error **errp) +{ + QObjectInputVisitor *qiv = to_qiv(v); + QString *qstr = qobject_to_qstring(qobject_input_get_object(qiv, name, + true)); + + parse_option_bool(name, qstr ? qstr->string : NULL, obj, errp); +} + static void qobject_input_type_str(Visitor *v, const char *name, char **obj, Error **errp) { @@ -312,8 +345,8 @@ static void qobject_input_type_str(Visitor *v, const char *name, char **obj, } static void qobject_input_type_number(Visitor *v, const char *name, double *obj, - Error **errp) -{ + Error **errp){ + QObjectInputVisitor *qiv = to_qiv(v); QObject *qobj = qobject_input_get_object(qiv, name, true); QInt *qint; @@ -335,6 +368,26 @@ static void qobject_input_type_number(Visitor *v, const char *name, double *obj, "number"); } +static void qobject_input_type_number_str(Visitor *v, const char *name, + double *obj, Error **errp) +{ + QObjectInputVisitor *qiv = to_qiv(v); + QString *qstr = qobject_to_qstring(qobject_input_get_object(qiv, name, + true)); + char *endp; + + if (qstr && qstr->string) { + errno = 0; + *obj = strtod(qstr->string, &endp); + if (errno == 0 && endp != qstr->string && *endp == '\0') { + return; + } + } + + error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null", + "number"); +} + static void qobject_input_type_any(Visitor *v, const char *name, QObject **obj, Error **errp) { @@ -356,6 +409,32 @@ static void qobject_input_type_null(Visitor *v, const char *name, Error **errp) } } +static void qobject_input_type_size_str(Visitor *v, const char *name, + uint64_t *obj, Error **errp) +{ + QObjectInputVisitor *qiv = to_qiv(v); + QString *qstr = qobject_to_qstring(qobject_input_get_object(qiv, name, + true)); + int64_t val; + char *endptr; + + if (qstr && qstr->string) { + val = qemu_strtosz_suffix(qstr->string, &endptr, + QEMU_STRTOSZ_DEFSUFFIX_B); + if (val < 0 || *endptr) { + error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name, + "a size value representible as a non-negative int64"); + return; + } + + *obj = val; + return; + } + + error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null", + "size"); +} + static void qobject_input_optional(Visitor *v, const char *name, bool *present) { QObjectInputVisitor *qiv = to_qiv(v); @@ -413,3 +492,35 @@ Visitor *qobject_input_visitor_new(QObject *obj, bool strict) return &v->visitor; } + +Visitor *qobject_string_input_visitor_new(QObject *obj) +{ + QObjectInputVisitor *v; + + v = g_malloc0(sizeof(*v)); + + v->visitor.type = VISITOR_INPUT; + v->visitor.start_struct = qobject_input_start_struct; + v->visitor.check_struct = qobject_input_check_struct; + v->visitor.end_struct = qobject_input_pop; + v->visitor.start_list = qobject_input_start_list; + v->visitor.next_list = qobject_input_next_list; + v->visitor.end_list = qobject_input_pop; + v->visitor.start_alternate = qobject_input_start_alternate; + v->visitor.type_int64 = qobject_input_type_int64_str; + v->visitor.type_uint64 = qobject_input_type_uint64_str; + v->visitor.type_bool = qobject_input_type_bool_str; + v->visitor.type_str = qobject_input_type_str; + v->visitor.type_number = qobject_input_type_number_str; + v->visitor.type_any = qobject_input_type_any; + v->visitor.type_null = qobject_input_type_null; + v->visitor.type_size = qobject_input_type_size_str; + v->visitor.optional = qobject_input_optional; + v->visitor.free = qobject_input_free; + v->strict = true; + + v->root = obj; + qobject_incref(obj); + + return &v->visitor; +} diff --git a/tests/test-qobject-input-visitor.c b/tests/test-qobject-input-visitor.c index 02895f0..1f60ad8 100644 --- a/tests/test-qobject-input-visitor.c +++ b/tests/test-qobject-input-visitor.c @@ -41,6 +41,7 @@ static void visitor_input_teardown(TestInputVisitorData *data, function so that the JSON string used by the tests are kept in the test functions (and not in main()). */ static Visitor *visitor_input_test_init_internal(TestInputVisitorData *data, + bool strict, bool autocast, const char *json_string, va_list *ap) { @@ -49,11 +50,31 @@ static Visitor *visitor_input_test_init_internal(TestInputVisitorData *data, data->obj = qobject_from_jsonv(json_string, ap); g_assert(data->obj); - data->qiv = qobject_input_visitor_new(data->obj, false); + if (autocast) { + assert(strict); + data->qiv = qobject_string_input_visitor_new(data->obj); + } else { + data->qiv = qobject_input_visitor_new(data->obj, strict); + } g_assert(data->qiv); return data->qiv; } +static GCC_FMT_ATTR(4, 5) +Visitor *visitor_input_test_init_full(TestInputVisitorData *data, + bool strict, bool autocast, + const char *json_string, ...) +{ + Visitor *v; + va_list ap; + + va_start(ap, json_string); + v = visitor_input_test_init_internal(data, strict, autocast, + json_string, &ap); + va_end(ap); + return v; +} + static GCC_FMT_ATTR(2, 3) Visitor *visitor_input_test_init(TestInputVisitorData *data, const char *json_string, ...) @@ -62,7 +83,8 @@ Visitor *visitor_input_test_init(TestInputVisitorData *data, va_list ap; va_start(ap, json_string); - v = visitor_input_test_init_internal(data, json_string, &ap); + v = visitor_input_test_init_internal(data, true, false, + json_string, &ap); va_end(ap); return v; } @@ -77,7 +99,8 @@ Visitor *visitor_input_test_init(TestInputVisitorData *data, static Visitor *visitor_input_test_init_raw(TestInputVisitorData *data, const char *json_string) { - return visitor_input_test_init_internal(data, json_string, NULL); + return visitor_input_test_init_internal(data, true, false, + json_string, NULL); } static void test_visitor_in_int(TestInputVisitorData *data, @@ -109,6 +132,33 @@ static void test_visitor_in_int_overflow(TestInputVisitorData *data, error_free_or_abort(&err); } +static void test_visitor_in_int_autocast(TestInputVisitorData *data, + const void *unused) +{ + int64_t res = 0, value = -42; + Visitor *v; + + v = visitor_input_test_init_full(data, true, true, + "\"-42\""); + + visit_type_int(v, NULL, &res, &error_abort); + g_assert_cmpint(res, ==, value); +} + +static void test_visitor_in_int_noautocast(TestInputVisitorData *data, + const void *unused) +{ + int64_t res = 0; + Visitor *v; + Error *err = NULL; + + v = visitor_input_test_init(data, "\"-42\""); + + visit_type_int(v, NULL, &res, &err); + g_assert(err != NULL); + error_free(err); +} + static void test_visitor_in_bool(TestInputVisitorData *data, const void *unused) { @@ -121,6 +171,32 @@ static void test_visitor_in_bool(TestInputVisitorData *data, g_assert_cmpint(res, ==, true); } +static void test_visitor_in_bool_autocast(TestInputVisitorData *data, + const void *unused) +{ + bool res = false; + Visitor *v; + + v = visitor_input_test_init_full(data, true, true, "\"yes\""); + + visit_type_bool(v, NULL, &res, &error_abort); + g_assert_cmpint(res, ==, true); +} + +static void test_visitor_in_bool_noautocast(TestInputVisitorData *data, + const void *unused) +{ + bool res = false; + Visitor *v; + Error *err = NULL; + + v = visitor_input_test_init(data, "\"true\""); + + visit_type_bool(v, NULL, &res, &err); + g_assert(err != NULL); + error_free(err); +} + static void test_visitor_in_number(TestInputVisitorData *data, const void *unused) { @@ -133,6 +209,58 @@ static void test_visitor_in_number(TestInputVisitorData *data, g_assert_cmpfloat(res, ==, value); } +static void test_visitor_in_number_autocast(TestInputVisitorData *data, + const void *unused) +{ + double res = 0, value = 3.14; + Visitor *v; + + v = visitor_input_test_init_full(data, true, true, "\"3.14\""); + + visit_type_number(v, NULL, &res, &error_abort); + g_assert_cmpfloat(res, ==, value); +} + +static void test_visitor_in_number_noautocast(TestInputVisitorData *data, + const void *unused) +{ + double res = 0; + Visitor *v; + Error *err = NULL; + + v = visitor_input_test_init(data, "\"3.14\""); + + visit_type_number(v, NULL, &res, &err); + g_assert(err != NULL); + error_free(err); +} + +static void test_visitor_in_size_autocast(TestInputVisitorData *data, + const void *unused) +{ + uint64_t res, value = 500 * 1024 * 1024; + Visitor *v; + + v = visitor_input_test_init_full(data, true, true, "\"500M\""); + + visit_type_size(v, NULL, &res, &error_abort); + g_assert_cmpfloat(res, ==, value); +} + +static void test_visitor_in_size_noautocast(TestInputVisitorData *data, + const void *unused) +{ + uint64_t res = 0; + Visitor *v; + Error *err = NULL; + + v = visitor_input_test_init(data, "\"500M\""); + + visit_type_size(v, NULL, &res, &err); + g_assert(err != NULL); + error_free(err); +} + static void test_visitor_in_string(TestInputVisitorData *data, const void *unused) { @@ -289,7 +417,7 @@ static void test_visitor_in_null(TestInputVisitorData *data, * when input is not null. */ - v = visitor_input_test_init(data, "{ 'a': null, 'b': '' }"); + v = visitor_input_test_init_full(data, false, false, "{ 'a': null, 'b': '' }"); visit_start_struct(v, NULL, NULL, 0, &error_abort); visit_type_null(v, "a", &error_abort); visit_type_str(v, "a", &tmp, &err); @@ -841,10 +969,26 @@ int main(int argc, char **argv) &in_visitor_data, test_visitor_in_int); input_visitor_test_add("/visitor/input/int_overflow", &in_visitor_data, test_visitor_in_int_overflow); + input_visitor_test_add("/visitor/input/int_autocast", + &in_visitor_data, test_visitor_in_int_autocast); + input_visitor_test_add("/visitor/input/int_noautocast", + &in_visitor_data, test_visitor_in_int_noautocast); input_visitor_test_add("/visitor/input/bool", &in_visitor_data, test_visitor_in_bool); + input_visitor_test_add("/visitor/input/bool_autocast", + &in_visitor_data, test_visitor_in_bool_autocast); + input_visitor_test_add("/visitor/input/bool_noautocast", + &in_visitor_data, test_visitor_in_bool_noautocast); input_visitor_test_add("/visitor/input/number", &in_visitor_data, test_visitor_in_number); + input_visitor_test_add("/visitor/input/number_autocast", + &in_visitor_data, test_visitor_in_number_autocast); + input_visitor_test_add("/visitor/input/number_noautocast", + &in_visitor_data, test_visitor_in_number_noautocast); + input_visitor_test_add("/visitor/input/size_autocast", + &in_visitor_data, test_visitor_in_size_autocast); + input_visitor_test_add("/visitor/input/size_noautocast", + &in_visitor_data, test_visitor_in_size_noautocast); input_visitor_test_add("/visitor/input/string", &in_visitor_data, test_visitor_in_string); input_visitor_test_add("/visitor/input/enum",