[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v9 05/37] vl: Improve use of qapi visitor
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH v9 05/37] vl: Improve use of qapi visitor |
Date: |
Thu, 21 Jan 2016 07:45:21 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
Eric Blake <address@hidden> writes:
> On 01/20/2016 06:43 AM, Markus Armbruster wrote:
>> Eric Blake <address@hidden> 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 :)
- Re: [Qemu-devel] [PATCH v9 07/37] qapi: Improve generated event use of qapi visitor, (continued)
[Qemu-devel] [PATCH v9 06/37] balloon: Improve use of qapi visitor, Eric Blake, 2016/01/19
[Qemu-devel] [PATCH v9 04/37] hmp: Improve use of qapi visitor, Eric Blake, 2016/01/19
[Qemu-devel] [PATCH v9 05/37] vl: Improve use of qapi visitor, Eric Blake, 2016/01/19
[Qemu-devel] [PATCH v9 03/37] qapi: Drop dead dealloc visitor variable, Eric Blake, 2016/01/19
[Qemu-devel] [PATCH v9 01/37] qobject: Document more shortcomings in our number handling, Eric Blake, 2016/01/19
[Qemu-devel] [PATCH v9 13/37] qom: Use typedef for Visitor, Eric Blake, 2016/01/19
[Qemu-devel] [PATCH v9 02/37] qapi: Avoid use of misnamed DO_UPCAST(), Eric Blake, 2016/01/19