Re: Questionable aspects of QEMU Error's design

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

27.04.2020 18:36, Markus Armbruster wrote:
> 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.
> 
> 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.
> 

OK, just let me know when to regenerate the series, it's not hard.

-- 
Best regards,
Vladimir