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

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.