Re: [Qemu-devel] [PATCH v10 10/25] qapi: Improve generated event use of qapi visitor

[Eric Blake]

On 02/01/2016 05:31 AM, Markus Armbruster wrote:

>> |+visit_start_struct(v, NULL, NULL, "ACPI_DEVICE_OST", 0, );
>> | if (err) {
>> | goto out;
>> | }
>> | visit_type_ACPIOSTInfo(v, , "info", );
>> | if (err) {
>> |-goto out;
>> |+goto out_obj;
>> | }
>> |-visit_end_struct(v, );
>> |+out_obj:
>> |+visit_end_struct(v, err ? NULL : );
> 
> Slightly awkward example, because out_obj is pointless in this
> degenerated case.  You could pick one with multiple members (thus
> multiple goto out_obj), or do pseudo-code hinting at multiple members.

DEVICE_DELETED, DEVICE_TRAY_MOVED, MEM_UNPLUG_ERROR,
NET_RX_FILTER_CHANGED, and SPICE_CONNECTED are nice candidates (two
members instead of one).  Do you want to take care of redoing any
portion of the commit message?

-- 
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 v10 10/25] qapi: Improve generated event use of qapi visitor

[Markus Armbruster]

Eric Blake  writes:

> 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, and pass NULL rather than "" as
> the 'kind' parameter (matching most other uses where obj is NULL).
>
> The changes to the generated code look like:
>
> | qov = qmp_output_visitor_new();
> |-g_assert(qov);
> |-
> | v = qmp_output_get_visitor(qov);
> |-g_assert(v);
> |
> |-/* Fake visit, as if all members are under a structure */
> |-visit_start_struct(v, NULL, "", "ACPI_DEVICE_OST", 0, );
> |+visit_start_struct(v, NULL, NULL, "ACPI_DEVICE_OST", 0, );
> | if (err) {
> | goto out;
> | }
> | visit_type_ACPIOSTInfo(v, , "info", );
> | if (err) {
> |-goto out;
> |+goto out_obj;
> | }
> |-visit_end_struct(v, );
> |+out_obj:
> |+visit_end_struct(v, err ? NULL : );

Slightly awkward example, because out_obj is pointless in this
degenerated case.  You could pick one with multiple members (thus
multiple goto out_obj), or do pseudo-code hinting at multiple members.

> | if (err) {
> | goto out;
> | }
> |
> | obj = qmp_output_get_qobject(qov);
> |-g_assert(obj != NULL);
> |+g_assert(obj);
> |
> | qdict_put_obj(qmp, "data", obj);
> | emit(QAPI_EVENT_ACPI_DEVICE_OST, qmp, );
> |
> |out:
> | qmp_output_visitor_cleanup(qov);
> | error_propagate(errp, err);
>
> Note that the 'goto out_obj' with no intervening code before the
> label, as well as the construct of 'err ? NULL : ', are both
> a bit unusual but also temporary; they get fixed in a later patch
> that splits visit_end_struct() to drop its errp parameter by moving
> some checking before the label.  But until that time, this was the
> simplest way to avoid the appearance of passing a possibly-set
> error to visit_end_struct(), even though actual code inspection
> shows that visit_end_struct() for a QMP output visitor will never
> set an error.
>
> Signed-off-by: Eric Blake 
>
> ---
> v10: avoid appearance of  misuse; enhance commit message
> v9: save churn in declaration order for later series on boxed params,
> drop Marc-Andre's R-b
> 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.

Patch looks good.

Re: [Qemu-devel] [PATCH v10 10/25] qapi: Improve generated event use of qapi visitor

[Markus Armbruster]

Eric Blake  writes:

> On 02/01/2016 05:31 AM, Markus Armbruster wrote:
>
>>> |+visit_start_struct(v, NULL, NULL, "ACPI_DEVICE_OST", 0, );
>>> | if (err) {
>>> | goto out;
>>> | }
>>> | visit_type_ACPIOSTInfo(v, , "info", );
>>> | if (err) {
>>> |-goto out;
>>> |+goto out_obj;
>>> | }
>>> |-visit_end_struct(v, );
>>> |+out_obj:
>>> |+visit_end_struct(v, err ? NULL : );
>> 
>> Slightly awkward example, because out_obj is pointless in this
>> degenerated case.  You could pick one with multiple members (thus
>> multiple goto out_obj), or do pseudo-code hinting at multiple members.
>
> DEVICE_DELETED, DEVICE_TRAY_MOVED, MEM_UNPLUG_ERROR,
> NET_RX_FILTER_CHANGED, and SPICE_CONNECTED are nice candidates (two
> members instead of one).  Do you want to take care of redoing any
> portion of the commit message?

Can do.  Unless something comes along that requires a respin.