Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com> writes: > 28.04.2020 08:20, Vladimir Sementsov-Ogievskiy wrote: >> 27.04.2020 18:36, Markus Armbruster wrote: >>> 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. >>> >> >> 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.