[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: Questionable aspects of QEMU Error's design
From: |
Markus Armbruster |
Subject: |
Re: Questionable aspects of QEMU Error's design |
Date: |
Fri, 03 Jul 2020 14:21:07 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) |
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
With my "[PATCH v2 00/44] Less clumsy error checking" applied, I now count
1946 void
879 signed integer
484 pointer
301 bool
33 unsigned integer
3 GuestFsfreezeStatus
1 gnutls_x509_crt_t
1 QCryptoCipherAlgorithm
1 COLOMessage
1 BlockdevDetectZeroesOptions
---------------------
3650 total
About 7% complete for function definitions.
> 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.
Different method this time: I count any direct function call that takes
&err other than &error_abort, &error_fatal, and whose value, if any, is
not used.
Current master: 1050
With my "[PATCH v2 00/44] Less clumsy error checking" applied: 649
About 38% complete for function 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
Top scorers master:
151 sysbus_realize()
34 qemu_opts_absorb_qdict()
29 visit_type_int()
24 visit_type_str()
23 cpu_exec_realizefn()
19 visit_type_size()
16 qdev_realize()
14 visit_type_bool()
12 visit_type_uint32()
11 visit_type_int32()
11 object_property_set_bool()
10 object_property_set_uint()
10 object_property_set_int()
+420 functions with fewer than 10 calls
Top scorers with my patches applied:
23 cpu_exec_realizefn()
15 visit_type_int()
10 visit_type_size()
+387 functions with fewer than 10 calls
Looks like this is going to be a long slog.
With functions into buckets by same name prefix up to the first '_':
63 visit
57 qmp
33 bdrv
29 cpu
26 xen
25 memory
+113 buckets with fewer than 25 calls
[...]
>
> Please understand these are rough numbers from quick & dirty scripts.
Still are.