| Message ID | 1461801715-24307-10-git-send-email-eblake@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Eric Blake <eblake@redhat.com> writes: > The qmp-input visitor was allowing callers to play rather fast > and loose: when visiting a QDict, you could grab members of the > root dictionary without first pushing into the dict; the final > such culprit was the QOM code for converting to and from object > properties. But we are about to tighten the input visitor, at > which point user_creatable_add_type() as called with a QMP input > visitor via qmp_object_add() MUST follow the same paradigms as > everyone else, of pushing into the struct before grabbing its > keys. > > The use of 'err ? NULL : &err' is temporary; a later patch will > clean that up when it splits visit_end_struct(). > > The change has no impact to the testsuite now, but is required to > avoid a failure in tests/test-netfilter once qmp-input is made > stricter to detect inconsistent 'name' arguments on the root visit. > > Since user_creatable_add_type() is also called with OptsVisitor > through user_creatable_add_opts(), we must also check that there > is no negative impact there; both pre- and post-patch, we see: > > $ ./x86_64-softmmu/qemu-system-x86_64 -nographic -nodefaults -qmp stdio -object secret,id=sec0,data=letmein,format=raw,foo=bar > qemu-system-x86_64: Property '.foo' not found Let's update the error message now that the error message regression is fixed. Can do on commit. > That is, the only new checking that the new visit_end_struct() can > perform is for excess input, but we already catch excess input > earlier in object_property_set(). Answers the question I had for v14. Didn't double check myself; I trust your analysis. > Signed-off-by: Eric Blake <eblake@redhat.com> > > --- > v15: hoist earlier in series, improve commit message > v14: no change > v13: no change > v12: new patch > --- > qom/object_interfaces.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c > index ab5da35..4a60d6d 100644 > --- a/qom/object_interfaces.c > +++ b/qom/object_interfaces.c > @@ -120,12 +120,20 @@ Object *user_creatable_add_type(const char *type, const char *id, > > obj = object_new(type); > if (qdict) { > + visit_start_struct(v, NULL, NULL, 0, &local_err); > + if (local_err) { > + goto out; > + } > for (e = qdict_first(qdict); e; e = qdict_next(qdict, e)) { > object_property_set(obj, v, e->key, &local_err); > if (local_err) { > - goto out; > + break; > } > } > + visit_end_struct(v, local_err ? NULL : &local_err); > + if (local_err) { > + goto out; > + } > } > > object_property_add_child(object_get_objects_root(), Hmm. How should this behave if !qdict? Compare: visit_start_struct(v, NULL, NULL, 0, &local_err); if (local_err) { goto out; } if (qdict) { for (e = qdict_first(qdict); e; e = qdict_next(qdict, e)) { object_property_set(obj, v, e->key, &local_err); if (local_err) { break; } } } visit_end_struct(v, local_err ? NULL : &local_err); if (local_err) { goto out; } The difference is that we get errors from visit_start_struct() and visit_end_struct() even when !qdict. However, both callers seem to pass non-null qdict. Should we sidestep the question by making that a precondition?
On 04/28/2016 08:46 AM, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> writes: > >> The qmp-input visitor was allowing callers to play rather fast >> and loose: when visiting a QDict, you could grab members of the >> root dictionary without first pushing into the dict; the final >> such culprit was the QOM code for converting to and from object >> properties. But we are about to tighten the input visitor, at >> which point user_creatable_add_type() as called with a QMP input >> visitor via qmp_object_add() MUST follow the same paradigms as >> everyone else, of pushing into the struct before grabbing its >> keys. >> >> >> $ ./x86_64-softmmu/qemu-system-x86_64 -nographic -nodefaults -qmp stdio -object secret,id=sec0,data=letmein,format=raw,foo=bar >> qemu-system-x86_64: Property '.foo' not found > > Let's update the error message now that the error message regression is > fixed. Can do on commit. I see when you fixed the error message regression your commits omitted data=letmein,format=raw; I included them here because they are marked mandatory in qapi, to make sure that we didn't have to think about whether excess arguments trump missing mandatory arguments in terms of error reporting. Up to you if you also feel comfortable shortening the command line to match the one in your commit message that fixed the regression. >> +++ b/qom/object_interfaces.c >> @@ -120,12 +120,20 @@ Object *user_creatable_add_type(const char *type, const char *id, >> >> obj = object_new(type); >> if (qdict) { >> + visit_start_struct(v, NULL, NULL, 0, &local_err); >> + if (local_err) { >> + goto out; >> + } >> for (e = qdict_first(qdict); e; e = qdict_next(qdict, e)) { >> object_property_set(obj, v, e->key, &local_err); >> if (local_err) { >> - goto out; >> + break; >> } >> } >> + visit_end_struct(v, local_err ? NULL : &local_err); >> + if (local_err) { >> + goto out; >> + } >> } >> >> object_property_add_child(object_get_objects_root(), > > Hmm. How should this behave if !qdict? Pre-existing. !qdict is not possible from qmp_object-add() (which throws QERR_INVALID_PARAMETER_TYPE on a missing or wrong QObject type for 'props'). Also not possible from user_creatable_add(). We could turn it into assert(qdict) and drop a level of indentation. > > However, both callers seem to pass non-null qdict. Should we sidestep > the question by making that a precondition? We came to the same conclusion, so yes :)
diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c index ab5da35..4a60d6d 100644 --- a/qom/object_interfaces.c +++ b/qom/object_interfaces.c @@ -120,12 +120,20 @@ Object *user_creatable_add_type(const char *type, const char *id, obj = object_new(type); if (qdict) { + visit_start_struct(v, NULL, NULL, 0, &local_err); + if (local_err) { + goto out; + } for (e = qdict_first(qdict); e; e = qdict_next(qdict, e)) { object_property_set(obj, v, e->key, &local_err); if (local_err) { - goto out; + break; } } + visit_end_struct(v, local_err ? NULL : &local_err); + if (local_err) { + goto out; + } } object_property_add_child(object_get_objects_root(),
The qmp-input visitor was allowing callers to play rather fast and loose: when visiting a QDict, you could grab members of the root dictionary without first pushing into the dict; the final such culprit was the QOM code for converting to and from object properties. But we are about to tighten the input visitor, at which point user_creatable_add_type() as called with a QMP input visitor via qmp_object_add() MUST follow the same paradigms as everyone else, of pushing into the struct before grabbing its keys. The use of 'err ? NULL : &err' is temporary; a later patch will clean that up when it splits visit_end_struct(). The change has no impact to the testsuite now, but is required to avoid a failure in tests/test-netfilter once qmp-input is made stricter to detect inconsistent 'name' arguments on the root visit. Since user_creatable_add_type() is also called with OptsVisitor through user_creatable_add_opts(), we must also check that there is no negative impact there; both pre- and post-patch, we see: $ ./x86_64-softmmu/qemu-system-x86_64 -nographic -nodefaults -qmp stdio -object secret,id=sec0,data=letmein,format=raw,foo=bar qemu-system-x86_64: Property '.foo' not found That is, the only new checking that the new visit_end_struct() can perform is for excess input, but we already catch excess input earlier in object_property_set(). Signed-off-by: Eric Blake <eblake@redhat.com> --- v15: hoist earlier in series, improve commit message v14: no change v13: no change v12: new patch --- qom/object_interfaces.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)