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

[Markus Armbruster <armbru@redhat.com>]

Eric Blake <eblake@redhat.com> writes:

> 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).

Makes sense, although I wonder why things work without
visit_end_struct().  Looking... I guess we skip visit_end_struct() only
on error, and then we go straight to opts_visitor_cleanup().  Okay,
that's sane enough to work.

But now I wonder whether requiring visit_end_struct() before
visit_cleanup() is a good idea.

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

I do that all the time for long messages :)