Re: [Qemu-devel] [PATCH v8 07/35] qapi: Improve generated event use of qapi visitor

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2878 bytes --]

On 01/05/2016 07:07 AM, Marc-André Lureau wrote:
> Hi
> 
> On Mon, Dec 21, 2015 at 6:08 PM, Eric Blake <eblake@redhat.com> wrote:
>> All other successful clients of visit_start_struct() were paired
>> with an unconditional visit_end_struct(); but the generated
>> code for events was relying on qmp_output_visitor_cleanup() to
>> work on an incomplete visit.  Alter the code to guarantee that
>> the struct is completed, which will make a future patch to
>> split visit_end_struct() easier to reason about.  While at it,
>> drop some assertions and comments that are not present in other
>> uses of the qmp output visitor, rearrange the declaration to
>> make it easier for a future patch to introduce the notion of
>> a boxed event visit, and pass NULL rather than "" as the 'kind'
>> parameter (matching most other uses where obj is NULL).
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>
>> ---
>> v8: no change
>> v7: place earlier in series, adjust handling of 'kind'
>> v6: new patch
>>
>> If desired, I can defer the hunk re-ordering the declaration of
>> obj to later in the series where it actually comes in handy.

See [1]

>> ---
>>  scripts/qapi-event.py | 14 ++++++--------
>>  scripts/qapi.py       |  5 +++--
>>  2 files changed, 9 insertions(+), 10 deletions(-)
>>
>> diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py
>> index 720486f..e37c07a 100644
>> --- a/scripts/qapi-event.py
>> +++ b/scripts/qapi-event.py
>> @@ -41,9 +41,9 @@ def gen_event_send(name, arg_type):
>>
>>      if arg_type and arg_type.members:
>>          ret += mcgen('''
>> +    QObject *obj;
>>      QmpOutputVisitor *qov;
>>      Visitor *v;
>> -    QObject *obj;
> 
> This looks like churning code.

This is my comment [1], and I am fine if we sink it to occur as part of
https://lists.gnu.org/archive/html/qemu-devel/2015-12/msg04394.html

>>
>> -    /* Fake visit, as if all members are under a structure */
>> -    visit_start_struct(v, NULL, "", "%(name)s", 0, &err);
>> +    visit_start_struct(v, NULL, NULL, "%(name)s", 0, &err);
> 
> The comment seemed somewhat useful to me.

The pattern visit_start_struct(v, NULL, NULL, name, 0, &err) occurs in
other places in the code without the comment, so it felt like a common
enough idiom that it didn't need special-casing here.  Also, even if we
want a comment, I didn't see a reason for the comment to be present in
the generated code multiple times (generated once per event); at most, a
simple Python '# comment' prior to the mcgen() would probably be
sufficient if we still think the comment adds anything.

> 
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> 

I guess when Markus gets back he can weigh in on it.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]