Eric Blake <ebl...@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 <ebl...@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 :)
