| Message ID | 1457571335-10938-7-git-send-email-eblake@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Eric Blake <eblake@redhat.com> writes: > Slightly rearrange the code in gen_event_send() for less generated > output, by initializing 'emit' sooner, deferring an assertion > to qdict_put_obj, and dropping a now-unused 'obj' local variable. > > While at it, document a FIXME related to the potential for a > compiler naming collision - if the user ever creates a QAPI event > whose name matches 'errp' or one of our local variables (like > 'emit'), we'll have to revisit how we generate functions to > avoid the problem. > > |@@ -25,16 +25,13 @@ void qapi_event_send_acpi_device_ost(ACP > | { > | QDict *qmp; > | Error *err = NULL; > |- QMPEventFuncEmit emit; > |+ QMPEventFuncEmit emit = qmp_event_get_func_emit(); > | QmpOutputVisitor *qov; > | Visitor *v; > |- QObject *obj; > | > |- emit = qmp_event_get_func_emit(); > | if (!emit) { > | return; > | } > |- > | qmp = qmp_event_build_dict("ACPI_DEVICE_OST"); > | > | qov = qmp_output_visitor_new(); > |@@ -53,11 +50,7 @@ out_obj: > | if (err) { > | goto out; > | } > |- > |- obj = qmp_output_get_qobject(qov); > |- g_assert(obj); We're not "deferring an assertion to qdict_put_obj()", we're dropping a dead one: qmp_output_get_qobject() never returns null. I figure the assertion dates back to the time when it still did. Back then, getting null here meant we screwed up. I just searched the code for similarly dead assertions. Found one in qapi_clone_InputEvent(), and serveral more in test-qmp-output-visitor.c. There's also an error return in qapi_copy_SocketAddress(). Useless? Should check for qnull instead? > |- > |- qdict_put_obj(qmp, "data", obj); > |+ qdict_put_obj(qmp, "data", qmp_output_get_qobject(qov)); > | emit(QAPI_EVENT_ACPI_DEVICE_OST, qmp, &err); > | > | out: > > Signed-off-by: Eric Blake <eblake@redhat.com> > > --- > v5: new patch > --- > scripts/qapi-event.py | 19 +++++++++---------- > 1 file changed, 9 insertions(+), 10 deletions(-) > > diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py > index c03cb78..02c9556 100644 > --- a/scripts/qapi-event.py > +++ b/scripts/qapi-event.py > @@ -29,13 +29,19 @@ def gen_event_send_decl(name, arg_type): > > > def gen_event_send(name, arg_type): > + # FIXME: Our declaration of local variables (and of 'errp' in the > + # parameter list) can collide with exploded members of the event's > + # data type passed in as parameters. If this collision ever hits in > + # practice, we can rename our local variables with a leading _ prefix, > + # or split the code into a wrapper function that creates a boxed > + # 'param' object then calls another to do the real work. > ret = mcgen(''' > > %(proto)s > { > QDict *qmp; > Error *err = NULL; > - QMPEventFuncEmit emit; > + QMPEventFuncEmit emit = qmp_event_get_func_emit(); > ''', > proto=gen_event_send_proto(name, arg_type)) > > @@ -43,16 +49,13 @@ def gen_event_send(name, arg_type): > ret += mcgen(''' > QmpOutputVisitor *qov; > Visitor *v; > - QObject *obj; > - Please keep the blank line here... > ''') > > ret += mcgen(''' > - emit = qmp_event_get_func_emit(); > + ... instead of adding it here. > if (!emit) { > return; > } > - Let's keep this one. > qmp = qmp_event_build_dict("%(name)s"); > > ''', > @@ -76,11 +79,7 @@ out_obj: > if (err) { > goto out; > } > - > - obj = qmp_output_get_qobject(qov); > - g_assert(obj); > - > - qdict_put_obj(qmp, "data", obj); > + qdict_put_obj(qmp, "data", qmp_output_get_qobject(qov)); > ''') > > ret += mcgen(''' Small improvements are welcome, too :)
On 03/10/2016 11:50 AM, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> writes: > >> Slightly rearrange the code in gen_event_send() for less generated >> output, by initializing 'emit' sooner, deferring an assertion >> to qdict_put_obj, and dropping a now-unused 'obj' local variable. >> >> While at it, document a FIXME related to the potential for a >> compiler naming collision - if the user ever creates a QAPI event >> whose name matches 'errp' or one of our local variables (like >> 'emit'), we'll have to revisit how we generate functions to >> avoid the problem. >> > > We're not "deferring an assertion to qdict_put_obj()", we're dropping a > dead one: qmp_output_get_qobject() never returns null. Oh, good point; I can improve the commit message. > > I figure the assertion dates back to the time when it still did. Back > then, getting null here meant we screwed up. > > I just searched the code for similarly dead assertions. Found one in > qapi_clone_InputEvent(), and serveral more in test-qmp-output-visitor.c. Speaking of that, I have a patch pending (but not yet posted) that adds a clone visitor, so that we don't need qapi_clone_InputEvent() (it's rather wasteful to convert into and back out of QObject when you can just directly clone). > > There's also an error return in qapi_copy_SocketAddress(). Useless? And that's the other hand-rolled clone that also gets nuked by my patch. Some obvious copy-and-paste between the two. > Should check for qnull instead? Not necessary; we can't return qnull unless we visit nothing (or, when my visit_type_null() lands, if we explicitly ask for it), but these callers are visiting something that is not null. >> %(proto)s >> { >> QDict *qmp; >> Error *err = NULL; >> - QMPEventFuncEmit emit; >> + QMPEventFuncEmit emit = qmp_event_get_func_emit(); >> ''', >> proto=gen_event_send_proto(name, arg_type)) >> >> @@ -43,16 +49,13 @@ def gen_event_send(name, arg_type): >> ret += mcgen(''' >> QmpOutputVisitor *qov; >> Visitor *v; >> - QObject *obj; >> - > > Please keep the blank line here... > >> ''') >> >> ret += mcgen(''' >> - emit = qmp_event_get_func_emit(); >> + > > ... instead of adding it here. Except that the next patch added one more declaration after Visitor *v, but not in direct text, where keeping the blank line unmoved would require splitting the mcgen() call into two parts. Or I could do ret += '\n'. > >> if (!emit) { >> return; >> } >> - > > Let's keep this one. Okay. > >> qmp = qmp_event_build_dict("%(name)s"); >> >> ''', >> @@ -76,11 +79,7 @@ out_obj: >> if (err) { >> goto out; >> } >> - >> - obj = qmp_output_get_qobject(qov); >> - g_assert(obj); >> - >> - qdict_put_obj(qmp, "data", obj); >> + qdict_put_obj(qmp, "data", qmp_output_get_qobject(qov)); >> ''') >> >> ret += mcgen(''' > > Small improvements are welcome, too :) >
diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py index c03cb78..02c9556 100644 --- a/scripts/qapi-event.py +++ b/scripts/qapi-event.py @@ -29,13 +29,19 @@ def gen_event_send_decl(name, arg_type): def gen_event_send(name, arg_type): + # FIXME: Our declaration of local variables (and of 'errp' in the + # parameter list) can collide with exploded members of the event's + # data type passed in as parameters. If this collision ever hits in + # practice, we can rename our local variables with a leading _ prefix, + # or split the code into a wrapper function that creates a boxed + # 'param' object then calls another to do the real work. ret = mcgen(''' %(proto)s { QDict *qmp; Error *err = NULL; - QMPEventFuncEmit emit; + QMPEventFuncEmit emit = qmp_event_get_func_emit(); ''', proto=gen_event_send_proto(name, arg_type)) @@ -43,16 +49,13 @@ def gen_event_send(name, arg_type): ret += mcgen(''' QmpOutputVisitor *qov; Visitor *v; - QObject *obj; - ''') ret += mcgen(''' - emit = qmp_event_get_func_emit(); + if (!emit) { return; } - qmp = qmp_event_build_dict("%(name)s"); ''', @@ -76,11 +79,7 @@ out_obj: if (err) { goto out; } - - obj = qmp_output_get_qobject(qov); - g_assert(obj); - - qdict_put_obj(qmp, "data", obj); + qdict_put_obj(qmp, "data", qmp_output_get_qobject(qov)); ''') ret += mcgen('''
Slightly rearrange the code in gen_event_send() for less generated output, by initializing 'emit' sooner, deferring an assertion to qdict_put_obj, and dropping a now-unused 'obj' local variable. While at it, document a FIXME related to the potential for a compiler naming collision - if the user ever creates a QAPI event whose name matches 'errp' or one of our local variables (like 'emit'), we'll have to revisit how we generate functions to avoid the problem. |@@ -25,16 +25,13 @@ void qapi_event_send_acpi_device_ost(ACP | { | QDict *qmp; | Error *err = NULL; |- QMPEventFuncEmit emit; |+ QMPEventFuncEmit emit = qmp_event_get_func_emit(); | QmpOutputVisitor *qov; | Visitor *v; |- QObject *obj; | |- emit = qmp_event_get_func_emit(); | if (!emit) { | return; | } |- | qmp = qmp_event_build_dict("ACPI_DEVICE_OST"); | | qov = qmp_output_visitor_new(); |@@ -53,11 +50,7 @@ out_obj: | if (err) { | goto out; | } |- |- obj = qmp_output_get_qobject(qov); |- g_assert(obj); |- |- qdict_put_obj(qmp, "data", obj); |+ qdict_put_obj(qmp, "data", qmp_output_get_qobject(qov)); | emit(QAPI_EVENT_ACPI_DEVICE_OST, qmp, &err); | | out: Signed-off-by: Eric Blake <eblake@redhat.com> --- v5: new patch --- scripts/qapi-event.py | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-)