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.
FYI, I'm working on converting QemuOpts, QAPI visitors and QOM. I keep
running into bugs. So far:
[PATCH v2 for-5.1 0/9] qemu-option: Fix corner cases and clean up
[PATCH for-5.1 0/5] qobject: Minor spring cleaning
[PATCH v2 00/14] Miscellaneous error handling fixes
[PATCH 0/4] Subject: [PATCH 0/4] smbus: SPD fixes
[PATCH 0/3] fuzz: Probably there is a better way to do this
[PATCH v2 00/15] qapi: Spring cleaning
[PATCH 00/11] More miscellaneous error handling fixes
I got another one coming for QOM and qdev before I can post the
conversion.
Vladimir, since the conversion will mess with error_propagate(), I'd
like to get it in before your auto-propagation work.
