Re: [Qemu-devel] [PATCH v9 08/37] qapi: Track all failures between visit_start/stop

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 01/20/2016 09:03 AM, Markus Armbruster wrote:
>> Eric Blake <address@hidden> writes:
>> 
>>> Inside the generated code between visit_start_struct() and
>>> visit_end_struct(), we were blindly setting the error into
>>> the caller's errp parameter.  But a future patch to split
>>> visit_end_struct() will require that we take action based
>>> on whether an error has occurred, which requires us to track
>>> all actions through a local err.  Rewrite the visits to be
>>> more in line with the other generated calls.
>>>
>
>>> |+    if (!*obj) {
>> 
>> err is clear here.
>> 
>>> |+        goto out_obj;
>>> |+    }
>>> |+    visit_type_GuestAgentCommandInfo_fields(v, obj, &err);
>>> |+out_obj:
>>> |+    error_propagate(errp, err);
>>> |+    err = NULL;
>> 
>> If we come from goto out_obj, these two lines are no-ops.
>
> I could move the label, if desired...
>
>> 
>>> |+    visit_end_struct(v, &err);
>>> |+out:
>>> |     error_propagate(errp, err);
>>> | }
>
>> 
>> gen_visit_union(), the other generator of visit_start_struct(), already
>> does it this way.  It generates additional goto out_obj, so the
>> placement of out_obj makes more sense there.  I guess we want to place
>> it in the same spot here to facilitate unifying the two.
>
> ...but as you observed, the unification in 31 is a bit easier with it
> placed identically.  Maybe a commit message comment is in order.

Or perhaps move it forward now, and move it back when you actually need
it back.

Reviewers appreciate miminal patch churn, but they also appreciate
patches making sense on their own.  Reviewer are looking at your patch
with precious little context in general, and next to zero visibility
into later patches in particular.