Re: [Qemu-devel] [PATCH 5/9] QemuOpts: Convert qemu_opts_foreach() to Error

[Eric Blake]

On 06/02/2015 05:33 AM, Markus Armbruster wrote:
> Eric Blake <address@hidden> writes:
> 
>> On 05/28/2015 06:21 AM, Markus Armbruster wrote:
>>> Retain the function value for now, to permit selective conversion of
>>> its callers.
>>>
>>> Signed-off-by: Markus Armbruster <address@hidden>
>>> ---

>>> -typedef int (*qemu_opts_loopfunc)(QemuOpts *opts, void *opaque);
>>> +typedef int (*qemu_opts_loopfunc)(void *opaque, QemuOpts *opts, Error 
>>> **errp);
>>
>> Might be nice to justify in the commit message why swapping the
>> arguments of the callback made sense.  But doesn't affect code
>> correctness, so:
>>
>> Reviewed-by: Eric Blake <address@hidden>
> 
> Not sure what to write.  Not even quite sure why I like it better this
> way, only that I do.  Could be because it puts the object we're
> modifying (when we modify any of the argument objects) on the left.

That's indeed weak, but it's more justification than nothing, so I'll
buy it.

>>>  int qemu_opts_foreach(QemuOptsList *list, qemu_opts_loopfunc func,
>>> -                      void *opaque)
>>> +                      void *opaque, Error **errp)
>>>  {
>>>      Location loc;
>>>      QemuOpts *opts;
>>> @@ -1062,7 +1063,7 @@ int qemu_opts_foreach(QemuOptsList *list, 
>>> qemu_opts_loopfunc func,
>>>      loc_push_none(&loc);
>>>      QTAILQ_FOREACH(opts, &list->head, next) {
>>>          loc_restore(&opts->loc);
>>> -        rc = func(opts, opaque);
>>> +        rc = func(opaque, opts, errp);
>>>          if (rc) {
>>>              return rc;
>>>          }
>>
>> Do you want to enforce that if errp is set, that rc is non-zero, to
>> match your contract?  Perhaps by assert(!*errp) at this point?  But if
>> the return value goes away later in the series in favor of only using
>> errp, it may be a moot question.
> 
> assert(!*errp) is incorrect, because callers may pass a null errp to
> ignore errors.  Could do
> 
>         if (rc) {
>             return rc;
>         }
>         assert(!errp || !*errp);
> 
> Catches misbehaving func() only when caller passes non-null errp.  To
> catcht it always, we'd need to use Error *err here, and
> error_propagate(errp, err).  I doubt it's worth the trouble.

Or, you could do:

if (!errp) {
    errp = &error_abort;
}
...
if (rc) {
    return rc;
}
assert(!*errp);

and not have to use error_propagate() - either the caller is tracking
errors, or the caller passed NULL because they are tracking return value
and we can assume that their callback will not raise errors (even on
negative returns).  But that's a slightly different contract.

You're right that the possibility of NULL makes it not as trivial as I
first thought, so I'm starting to waffle on whether enforcing the
contract is worth the extra lines of code.  Anyone else with an opinion?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature