Re: Questionable aspects of QEMU Error's design

[Markus Armbruster <armbru@redhat.com>]

Markus Armbruster <armbru@redhat.com> writes:

> Markus Armbruster <armbru@redhat.com> writes:
>
>> QEMU's Error was patterned after GLib's GError.  Differences include:
> [...]
>> * Return value conventions
>>
>>   Common: non-void functions return a distinct error value on failure
>>   when such a value can be defined.  Patterns:
>>
>>   - Functions returning non-null pointers on success return null pointer
>>     on failure.
>>
>>   - Functions returning non-negative integers on success return a
>>     negative error code on failure.
>>
>>   Different: GLib discourages void functions, because these lead to
>>   awkward error checking code.  We have tons of them, and tons of
>>   awkward error checking code:
>>
>>     Error *err = NULL;
>>     frobnicate(arg, &err);
>>     if (err) {
>>         ... recover ...
>>         error_propagate(errp, err);
>>     }
>>
>>   instead of
>>
>>     if (!frobnicate(arg, errp))
>>         ... recover ...
>>     }
>>
>>   Can also lead to pointless creation of Error objects.
>>
>>   I consider this a design mistake.  Can we still fix it?  We have more
>>   than 2000 void functions taking an Error ** parameter...
>>
>>   Transforming code that receives and checks for errors with Coccinelle
>>   shouldn't be hard.  Transforming code that returns errors seems more
>>   difficult.  We need to transform explicit and implicit return to
>>   either return true or return false, depending on what we did to the
>>   @errp parameter on the way to the return.  Hmm.
> [...]
>
> To figure out what functions with an Error ** parameter return, I used
> Coccinelle to find such function definitions and print the return types.
> Summary of results:
>
>    2155 void
>     873 signed integer
>     494 pointer
>     153 bool
>      33 unsigned integer
>       6 enum
>    ---------------------
>    3714 total
>
> I then used Coccinelle to find checked calls of void functions (passing
> &error_fatal or &error_abort is not considered "checking" here).  These
> calls become simpler if we make the functions return a useful value.  I
> found a bit under 600 direct calls, and some 50 indirect calls.
>
> Most frequent direct calls:
>
>     127 object_property_set_bool
>      27 qemu_opts_absorb_qdict
>      16 visit_type_str
>      14 visit_type_int
>      10 visit_type_uint32
>
> Let's have a closer look at object_property_set() & friends.  Out of
> almost 1000 calls, some 150 are checked.  While I'm sure many of the
> unchecked calls can't actually fail, I am concerned some unchecked calls
> can.
>
> If we adopt the convention to return a value that indicates success /
> failure, we should consider converting object.h to it sooner rather than
> later.
>
> Please understand these are rough numbers from quick & dirty scripts.

Paolo, Daniel, Eduardo,

Please pick one for QOM:

* Do nothing.  Looks like

     object_property_set_bool(..., &err);
     if (err) {
         error_propagate(errp, err);
         return;
     }

* Return true on success, false on error.  Looks like

     if (!object_property_set_bool(..., errp)) {
         return;
     }

* Return 0 on success, -1 on error.  Looks like

     if (object_property_set_bool(..., errp) < 0) {
         return;
     }

  This is slightly more likely to require line wrapping than the
  previous one.