[Top][All Lists]

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

Re: [PATCH v6] hw/core/qdev: cleanup Error ** variables

From: Eric Blake
Subject: Re: [PATCH v6] hw/core/qdev: cleanup Error ** variables
Date: Thu, 5 Dec 2019 10:45:26 -0600
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.2.2

On 12/5/19 8:48 AM, Vladimir Sementsov-Ogievskiy wrote:

@@ -918,27 +917,26 @@ static void device_set_realized(Object *obj, bool value, 
Error **errp)
} else if (!value && dev->realized) {
-        Error **local_errp = NULL;
+        /* We want local_err to track only the first error */
           QLIST_FOREACH(bus, &dev->child_bus, sibling) {
-            local_errp = local_err ? NULL : &local_err;
               object_property_set_bool(OBJECT(bus), false, "realized",
-                                     local_errp);
+                                     local_err ? NULL : &local_err);

This is a rather unusual way to keep the first error of several.

It may be unusual, but has the benefit of avoiding error_propagate...

qapi/error.h advises:

   * Receive and accumulate multiple errors (first one wins):
   *     Error *err = NULL, *local_err = NULL;
   *     foo(arg, &err);
   *     bar(arg, &local_err);
   *     error_propagate(&err, local_err);
   *     if (err) {
   *         handle the error...
   *     }

Hmm, honestly, I like more what I've written:

1. less code
2. logic is more clean: we store first error to local_err, and after first error
     pass NULL as a parameter. No propagation or extra error variables.
3. more efficient (no propagation, no extra allocation for errors which we'll 
     anyway) (I understand that efficiency of error path is not thing to care 
     so it's at third place)

Also, propagation which you propose is also unusual thing (it proposed in 
but who reads it :). I've never seen it before, and I've to go and check that
error_propagate works correctly when first argument is already set.

So, I'd prefer to keep now this patch as is, and to convert later if we really 
need it.

If replacing this by the usual way is too troublesome now, we can do it
after the ERRP_AUTO_PROPAGATE() conversion.  Your choice.

...and after conversion to use ERRP_AUTO_PROPATATE(), the use of error_propagate() should NOT occur in any code _except_ for the macro definition (any other use of the function points out a place where we failed to use the macro to get rid of boilerplate).

Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

reply via email to

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