Markus Armbruster <arm...@redhat.com> writes:
> Markus Armbruster <arm...@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.
