Message ID | 1460131992-32278-7-git-send-email-eblake@redhat.com |
---|---|
State | New |
Headers | show |
Eric Blake <eblake@redhat.com> writes: > Commit e8316d7 mistakenly passed consume=true in qmp_input_optional(), right? > when checking if > an optional member was present, but the mistake was silently > ignored since the code happily let us extract a member more than > once. Tighten up the input visitor to ensure that a member is > consumed exactly once. To keep the testsuite happy in the case > of incomplete input, we have to check whether a member exists > in the dictionary before trying to remove it. Sure this is only for the testsuite's benefit? You fix commit e8316d7's incorrect consume=true, don't you? Recommend to mention that explicitly. > > Signed-off-by: Eric Blake <eblake@redhat.com> > > --- > v14: no change > v13: no change > v12: new patch > --- > qapi/qmp-input-visitor.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c > index 7ba6d3d..cfaf378 100644 > --- a/qapi/qmp-input-visitor.c > +++ b/qapi/qmp-input-visitor.c > @@ -65,10 +65,12 @@ static QObject *qmp_input_get_object(QmpInputVisitor *qiv, > /* If we have a name, and we're in a dictionary, then return that > * value. */ > if (name && qobject_type(qobj) == QTYPE_QDICT) { > - if (tos->h && consume) { > - g_hash_table_remove(tos->h, name); > + qobj = qdict_get(qobject_to_qdict(qobj), name); > + if (tos->h && consume && qobj) { > + bool removed = g_hash_table_remove(tos->h, name); > + assert(removed); > } > - return qdict_get(qobject_to_qdict(qobj), name); > + return qobj; > } > > /* If we are in the middle of a list, then return the next element > @@ -338,7 +340,7 @@ static void qmp_input_type_any(Visitor *v, const char *name, QObject **obj, > static void qmp_input_optional(Visitor *v, const char *name, bool *present) > { > QmpInputVisitor *qiv = to_qiv(v); > - QObject *qobj = qmp_input_get_object(qiv, name, true); > + QObject *qobj = qmp_input_get_object(qiv, name, false); > > if (!qobj) { > *present = false;
On 04/13/2016 10:06 AM, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> writes: > >> Commit e8316d7 mistakenly passed consume=true > > in qmp_input_optional(), right? yes > >> when checking if >> an optional member was present, but the mistake was silently >> ignored since the code happily let us extract a member more than >> once. Tighten up the input visitor to ensure that a member is >> consumed exactly once. [1] by fixing qmp_input_optional() to pass consume=false >> To keep the testsuite happy in the case >> of incomplete input, we have to check whether a member exists >> in the dictionary before trying to remove it. > > Sure this is only for the testsuite's benefit? The testsuite was the only client that failed under the tighter semantics; but the better semantics allow later patches to further improve the code while guaranteeing that clients remain sane. > > You fix commit e8316d7's incorrect consume=true, don't you? Recommend > to mention that explicitly. I thought I did, but I can add wording [1] along those lines.
Eric Blake <eblake@redhat.com> writes: > On 04/13/2016 10:06 AM, Markus Armbruster wrote: >> Eric Blake <eblake@redhat.com> writes: >> >>> Commit e8316d7 mistakenly passed consume=true >> >> in qmp_input_optional(), right? > > yes > >> >>> when checking if >>> an optional member was present, but the mistake was silently >>> ignored since the code happily let us extract a member more than >>> once. Tighten up the input visitor to ensure that a member is >>> consumed exactly once. > > [1] by fixing qmp_input_optional() to pass consume=false > >>> To keep the testsuite happy in the case >>> of incomplete input, we have to check whether a member exists >>> in the dictionary before trying to remove it. >> >> Sure this is only for the testsuite's benefit? > > The testsuite was the only client that failed under the tighter > semantics; but the better semantics allow later patches to further > improve the code while guaranteeing that clients remain sane. Do we know that non-testsuite code can't fail for some input? >> >> You fix commit e8316d7's incorrect consume=true, don't you? Recommend >> to mention that explicitly. > > I thought I did, but I can add wording [1] along those lines.
diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c index 7ba6d3d..cfaf378 100644 --- a/qapi/qmp-input-visitor.c +++ b/qapi/qmp-input-visitor.c @@ -65,10 +65,12 @@ static QObject *qmp_input_get_object(QmpInputVisitor *qiv, /* If we have a name, and we're in a dictionary, then return that * value. */ if (name && qobject_type(qobj) == QTYPE_QDICT) { - if (tos->h && consume) { - g_hash_table_remove(tos->h, name); + qobj = qdict_get(qobject_to_qdict(qobj), name); + if (tos->h && consume && qobj) { + bool removed = g_hash_table_remove(tos->h, name); + assert(removed); } - return qdict_get(qobject_to_qdict(qobj), name); + return qobj; } /* If we are in the middle of a list, then return the next element @@ -338,7 +340,7 @@ static void qmp_input_type_any(Visitor *v, const char *name, QObject **obj, static void qmp_input_optional(Visitor *v, const char *name, bool *present) { QmpInputVisitor *qiv = to_qiv(v); - QObject *qobj = qmp_input_get_object(qiv, name, true); + QObject *qobj = qmp_input_get_object(qiv, name, false); if (!qobj) { *present = false;
Commit e8316d7 mistakenly passed consume=true when checking if an optional member was present, but the mistake was silently ignored since the code happily let us extract a member more than once. Tighten up the input visitor to ensure that a member is consumed exactly once. To keep the testsuite happy in the case of incomplete input, we have to check whether a member exists in the dictionary before trying to remove it. Signed-off-by: Eric Blake <eblake@redhat.com> --- v14: no change v13: no change v12: new patch --- qapi/qmp-input-visitor.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)