Re: [Qemu-devel] [PATCH v9 05/37] vl: Improve use of qapi visitor

[Eric Blake <eblake@redhat.com>]

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

On 01/20/2016 06:43 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> Cache the visitor in a local variable instead of repeatedly
>> calling the accessor.  Pass NULL for the visit_start_struct()
>> object (which matches the fact that we were already passing 0
>> for the size argument, because we aren't using the visit to
>> allocate a qapi struct).  Guarantee that visit_end_struct()
>> is called if visit_start_struct() succeeded.
> 
> Three separate things --- you're pushing it now :)

Two of them the same as in 4/37.

So, among the five items, I did a split in two based on file.  I could
split in the other direction:

fix hmp and vl to cache their visitor
fix hmp and vl to pass NULL to avoid pointless allocation
fix vl to guarantee visit_end_struct

> 
> Impact of not calling visit_end_struct()?

Assertion failures after patch 24.  :)
Basically, I wanted to see whether the code base could get simpler if we
always enforced visit start/end grouping, and there were only a couple
of outliers that needed fixing (here, and in patch 7).


>>
>>      qdict_del(pdict, "qom-type");
>> -    visit_type_str(opts_get_visitor(ov), &type, "qom-type", &err);
>> +    visit_type_str(v, &type, "qom-type", &err);
>>      if (err) {
>>          goto out;
>>      }
> 
> If we get here, we must call visit_end_struct().
> 
>>      if (!type_predicate(type)) {
>> +        visit_end_struct(v, NULL);
> 
> Here, you add the previously missing visit_end_struct() to the error
> path.
> 
>>          goto out;
>>      }
>>
>>      qdict_del(pdict, "id");
>> -    visit_type_str(opts_get_visitor(ov), &id, "id", &err);
>> +    visit_type_str(v, &id, "id", &err);
>>      if (err) {
>> -        goto out;
>> +        goto out_end;
> 
> Here, you skip to the function's cleanup, which now includes
> visit_end_struct().
> 
> Such a mix is can be a sign the cleanup isn't quite in the right order.
> 
> When we take this error path to out_end, then:
> 
>    out_end:
>        visit_end_struct(v, &err_end);   // called, as we must
>        if (!err && err_end) {           // !err is false
>            qmp_object_del(id, NULL);    // not called
>        }
>        error_propagate(&err, err_end);  // since err, this is
>                                         // error_free(err_end)

Correct. And it gets simpler later on, when visit_end_struct() loses the
errp argument (patch 33).

> 
>>      }
>>
>> -    object_add(type, id, pdict, opts_get_visitor(ov), &err);
>> -    if (err) {
>> -        goto out;
>> -    }
>> -    visit_end_struct(opts_get_visitor(ov), &err);
>> -    if (err) {
>> +    object_add(type, id, pdict, v, &err);
>> +
>> +out_end:
>> +    visit_end_struct(v, &err_end);
> 
> visit_end_struct() must be called when visit_start_struct() succeeded.
> All error paths after that success pass through here, except for one,
> and that one calls visit_end_struct().  Okay.
> 
>> +    if (!err && err_end) {
>>          qmp_object_del(id, NULL);
>>      }
> 
> qmp_object_del() must be called when we fail after object_add()
> succeeded.  That's what the condition does.
> 
>> +    error_propagate(&err, err_end);
>>
>>  out:
> 
> Cleanup below here must be done always.
> 
>>      opts_visitor_cleanup(ov);
> 
> The only reason I'm not asking you to rewrite this mess is that you're
> already rewriting it later in this series.

So I think you found patch 33.

> 
>> @@ -2867,7 +2870,6 @@ out:
>>      QDECREF(pdict);
>>      g_free(id);
>>      g_free(type);
>> -    g_free(dummy);
>>      if (err) {
>>          error_report_err(err);
>>          return -1;
> 
> I wonder whether splitting this and the previous patch along the change
> rather than the file would come out nicer:
> 
> 1. Cache the visitor
> 
> 2. Suppress the pointless allocation
> 
> 3. Fix to call visit_end_struct()
> 

What do you know - we're thinking on the same lines :)  [And now you
know I just replied to your email in the order that I read it, rather
than reading the whole thing first]

-- 
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 --]