On 01/20/2016 06:43 AM, Markus Armbruster wrote: > Eric Blake <ebl...@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
signature.asc
Description: OpenPGP digital signature
