[v9,05/37] vl: Improve use of qapi visitor

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.

Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

---
v9: no change
v8: no change
v7: place earlier in series, drop attempts to provide a 'kind' string,
drop bogus avoidance of qmp_object_del() on error
v6: new patch, split from RFC on v5 7/46
---
 vl.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

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

Impact of not calling visit_end_struct()?

> Signed-off-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> ---
> v9: no change
> v8: no change
> v7: place earlier in series, drop attempts to provide a 'kind' string,
> drop bogus avoidance of qmp_object_del() on error
> v6: new patch, split from RFC on v5 7/46
> ---
>  vl.c | 26 ++++++++++++++------------
>  1 file changed, 14 insertions(+), 12 deletions(-)
>
> diff --git a/vl.c b/vl.c
> index f043009..aaa5403 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2822,44 +2822,47 @@ static bool object_create_delayed(const char *type)
>  static int object_create(void *opaque, QemuOpts *opts, Error **errp)
>  {
>      Error *err = NULL;
> +    Error *err_end = NULL;
>      char *type = NULL;
>      char *id = NULL;
> -    void *dummy = NULL;
>      OptsVisitor *ov;
>      QDict *pdict;
>      bool (*type_predicate)(const char *) = opaque;
> +    Visitor *v;
>
>      ov = opts_visitor_new(opts);
>      pdict = qemu_opts_to_qdict(opts, NULL);
> +    v = opts_get_visitor(ov);
>
> -    visit_start_struct(opts_get_visitor(ov), &dummy, NULL, NULL, 0, &err);
> +    visit_start_struct(v, NULL, NULL, NULL, 0, &err);

Same argument as for the previous patch.

>      if (err) {
>          goto out;
>      }
>
>      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)

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

> @@ -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()

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