qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: Questionable aspects of QEMU Error's design


From: Vladimir Sementsov-Ogievskiy
Subject: Re: Questionable aspects of QEMU Error's design
Date: Thu, 14 May 2020 10:59:47 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.8.0

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?

--
Best regards,
Vladimir



reply via email to

[Prev in Thread] Current Thread [Next in Thread]