[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, 15 May 2020 06:28:37 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) |
Vladimir Sementsov-Ogievskiy <address@hidden> writes:
> 28.04.2020 08:20, Vladimir Sementsov-Ogievskiy wrote:
>> 27.04.2020 18:36, Markus Armbruster wrote:
>>> Markus Armbruster <address@hidden> writes:
>>>
>>>> Markus Armbruster <address@hidden> 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.
>>
>
> Hi! Is all that merged? Should I resend now?
I ran into many bugs and fell into a few rabbit holes. I'm busy
finishing and flushing the patches.