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

[Markus Armbruster]

Eric Blake <address@hidden> 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, &err);
> |+    visit_start_struct(v, NULL, NULL, "ACPI_DEVICE_OST", 0, &err);
> |     if (err) {
> |         goto out;
> |     }
> |     visit_type_ACPIOSTInfo(v, &info, "info", &err);
> |     if (err) {
> |-        goto out;
> |+        goto out_obj;
> |     }
> |-    visit_end_struct(v, &err);
> |+out_obj:
> |+    visit_end_struct(v, err ? NULL : &err);

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, &err);
> |
> |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 : &err', 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 <address@hidden>
>
> ---
> v10: avoid appearance of &err 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.