Re: [Qemu-devel] [PATCH v5 06/14] qapi-event: Slightly shrink generated code

[Markus Armbruster]

Eric Blake  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.

I think this should go into the next patch, where we actually fix the
same collision in command marshallers.

> |@@ -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, );
> |
> | out:

This is actually two separate simplifications.  One is initializing emit
instead of assigning to it:

> |@@ -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();

I'm afraid I don't like it, because it separates
qmp_event_get_func_emit() from its check.

A more likable simplification would be dropping the indirection
outright: emit is always monitor_qapi_event_queue(), except in
test-qmp-event.  That's a rather weak excuse for complicating things
with an indirection.  Let's not upset this series with it, though.

The other simplification is burying the dead check of
qmp_output_get_qobject()'s value:

> |@@ -25,16 +25,13 @@ void qapi_event_send_acpi_device_ost(ACP
> | {
> | QDict *qmp;
> | Error *err = NULL;
> | QMPEventFuncEmit emit;
> | QmpOutputVisitor *qov;
> | Visitor *v;
> |-QObject *obj;
> |
> | emit = qmp_event_get_func_emit();
> |@@ -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, );
> |
> | out:

Yes, please.

Patch shrinks to just this:

>From bfaa962b04f5365289c46dadb0c078a13d52eae8 Mon Sep 17 00:00:00 2001
From: Eric Blake 
Date: Wed, 9 Mar 2016 17:55:27 -0700
Subject: [PATCH 06/14] qapi-event: Drop qmp_output_get_qobject() null check

qmp_output_get_qobject() was changed never to return null some time
ago, but the qapi_event_send_FOO() functions still check.  Clean that
up:

|@@ -28,7 +28,6 @@ void qapi_event_send_acpi_device_ost(ACP
| QMPEventFuncEmit emit;
| QmpOutputVisitor *qov;
| Visitor *v;
|-QObject *obj;
|
| emit = qmp_event_get_func_emit();
| if (!emit) {
|@@ -54,10 +53,7 @@ out_obj:
| 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, );
|
| out:

Signed-off-by: Eric Blake 
---
 scripts/qapi-event.py | 6 +-
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py
index c03cb78..27af206 100644
--- a/scripts/qapi-event.py
+++ b/scripts/qapi-event.py
@@ -43,7 +43,6 @@ def gen_event_send(name, arg_type):
 ret += mcgen('''
 QmpOutputVisitor *qov;
 Visitor *v;
-QObject *obj;
 
 ''')
 
@@ -77,10 +76,7 @@ out_obj:
 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('''
-- 
2.4.3

Re: [Qemu-devel] [PATCH v5 06/14] qapi-event: Slightly shrink generated code

[Eric Blake]

On 03/10/2016 11:50 AM, Markus Armbruster wrote:
> Eric Blake  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 :)
> 

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v5 06/14] qapi-event: Slightly shrink generated code

[Markus Armbruster]

Eric Blake  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, );
> |
> | out:
>
> Signed-off-by: Eric Blake 
>
> ---
> 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 :)

[Qemu-devel] [PATCH v5 06/14] qapi-event: Slightly shrink generated code

[Eric Blake]

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, );
|
| out:

Signed-off-by: Eric Blake 

---
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;
-
 ''')

 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('''
-- 
2.5.0