diff mbox

[v5,2/4] qdev: using NULL instead of local_err for qbus_child unrealize

Message ID 1409659388-9404-3-git-send-email-arei.gonglei@huawei.com
State New
Headers show

Commit Message

Gonglei (Arei) Sept. 2, 2014, 12:03 p.m. UTC 
From: Gonglei <arei.gonglei@huawei.com>

Forcefully unrealize all children regardless of early-iteration
errors(if happened). We should keep going with cleanup operation
rather than report a error immediately, meanwhile store the first
child unrealize failure and propagate it at the end.

We also forcefully un-vmsding and unrealizing actual object too.
Because we can not guarantee that a botched recursive has no
side-effects.

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 hw/core/qdev.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

Comments

Peter Crosthwaite Sept. 3, 2014, 1:08 p.m. UTC | #1 
On Tue, Sep 2, 2014 at 10:03 PM,  <arei.gonglei@huawei.com> wrote:
> From: Gonglei <arei.gonglei@huawei.com>
>
> Forcefully unrealize all children regardless of early-iteration
> errors(if happened). We should keep going with cleanup operation
> rather than report a error immediately, meanwhile store the first
> child unrealize failure and propagate it at the end.
>
> We also forcefully un-vmsding and unrealizing actual object too.
> Because we can not guarantee that a botched recursive has no
> side-effects.
>

This last sentance doesn't make too much sense to me, but the rest of
the commit message covers it nicely so I think you can drop it.

> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> ---
>  hw/core/qdev.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 4a1ac5b..c869520 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -813,6 +813,7 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
>      DeviceClass *dc = DEVICE_GET_CLASS(dev);
>      BusState *bus;
>      Error *local_err = NULL;
> +    Error *child_unrealized_err = NULL;
>
>      if (dev->hotplugged && !dc->hotpluggable) {
>          error_set(errp, QERR_DEVICE_NO_HOTPLUG, object_get_typename(obj));
> @@ -875,13 +876,17 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
>              object_property_set_bool(OBJECT(bus), false, "realized",
>                                       &local_err);

I think you can make the concept of "pass in a errp only if I don't
have one already" simpler with this pattern:

Error **local_errp = local_err ? NULL : &local_err;
object_property_set_bool(... , local_errp)

Or if your into one-line killers you could do the ?: inline (although
some reviewers don't like that).

>              if (local_err != NULL) {
> -                break;
> +                if (child_unrealized_err == NULL) {
> +                    child_unrealized_err = error_copy(local_err);
> +                }
> +                error_free(local_err);
> +                local_err = NULL;
>              }

Then this whole if can go ...

>          }
> -        if (qdev_get_vmsd(dev) && local_err == NULL) {
> +        if (qdev_get_vmsd(dev)) {
>              vmstate_unregister(dev, qdev_get_vmsd(dev), dev);
>          }
> -        if (dc->unrealize && local_err == NULL) {
> +        if (dc->unrealize) {

local_errp = local_err ? NULL : &local_err;

>              dc->unrealize(dev, &local_err);
>          }
>          dev->pending_deleted_event = true;
> @@ -889,6 +894,10 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
>
>      if (local_err != NULL) {
>          error_propagate(errp, local_err);
> +        error_free(child_unrealized_err);
> +        return;
> +    } else if (child_unrealized_err != NULL) {
> +        error_propagate(errp, child_unrealized_err);

Then this if-else is not needed either.

Otherwise,

Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

Regards,
Peter

>          return;
>      }
>
> --
> 1.7.12.4
>
>
>
Gonglei (Arei) Sept. 4, 2014, 12:58 a.m. UTC | #2 
> From: peter.crosthwaite@petalogix.com

> [mailto:peter.crosthwaite@petalogix.com] On Behalf Of Peter Crosthwaite

> Sent: Wednesday, September 03, 2014 9:09 PM

g NULL instead of local_err
> for qbus_child unrealize

> 

> On Tue, Sep 2, 2014 at 10:03 PM,  <arei.gonglei@huawei.com> wrote:

> > From: Gonglei <arei.gonglei@huawei.com>

> >

> > Forcefully unrealize all children regardless of early-iteration

> > errors(if happened). We should keep going with cleanup operation

> > rather than report a error immediately, meanwhile store the first

> > child unrealize failure and propagate it at the end.

> >

> > We also forcefully un-vmsding and unrealizing actual object too.

> > Because we can not guarantee that a botched recursive has no

> > side-effects.

> >

> 

> This last sentance doesn't make too much sense to me, but the rest of

> the commit message covers it nicely so I think you can drop it.

> 

OK.

> > Signed-off-by: Gonglei <arei.gonglei@huawei.com>

> > ---

> >  hw/core/qdev.c | 15 ++++++++++++---

> >  1 file changed, 12 insertions(+), 3 deletions(-)

> >

> > diff --git a/hw/core/qdev.c b/hw/core/qdev.c

> > index 4a1ac5b..c869520 100644

> > --- a/hw/core/qdev.c

> > +++ b/hw/core/qdev.c

> > @@ -813,6 +813,7 @@ static void device_set_realized(Object *obj, bool

> value, Error **errp)

> >      DeviceClass *dc = DEVICE_GET_CLASS(dev);

> >      BusState *bus;

> >      Error *local_err = NULL;

> > +    Error *child_unrealized_err = NULL;

> >

> >      if (dev->hotplugged && !dc->hotpluggable) {

> >          error_set(errp, QERR_DEVICE_NO_HOTPLUG,

> object_get_typename(obj));

> > @@ -875,13 +876,17 @@ static void device_set_realized(Object *obj, bool

> value, Error **errp)

> >              object_property_set_bool(OBJECT(bus), false, "realized",

> >                                       &local_err);

> 

> I think you can make the concept of "pass in a errp only if I don't

> have one already" simpler with this pattern:

> 

> Error **local_errp = local_err ? NULL : &local_err;

> object_property_set_bool(... , local_errp)

> 

OK, this is a good idea!

> Or if your into one-line killers you could do the ?: inline (although

> some reviewers don't like that).

> 

> >              if (local_err != NULL) {

> > -                break;

> > +                if (child_unrealized_err == NULL) {

> > +                    child_unrealized_err = error_copy(local_err);

> > +                }

> > +                error_free(local_err);

> > +                local_err = NULL;

> >              }

> 

> Then this whole if can go ...

> 

> >          }

> > -        if (qdev_get_vmsd(dev) && local_err == NULL) {

> > +        if (qdev_get_vmsd(dev)) {

> >              vmstate_unregister(dev, qdev_get_vmsd(dev), dev);

> >          }

> > -        if (dc->unrealize && local_err == NULL) {

> > +        if (dc->unrealize) {

> 

> local_errp = local_err ? NULL : &local_err;

> 

> >              dc->unrealize(dev, &local_err);

> >          }

> >          dev->pending_deleted_event = true;

> > @@ -889,6 +894,10 @@ static void device_set_realized(Object *obj, bool

> value, Error **errp)

> >

> >      if (local_err != NULL) {

> >          error_propagate(errp, local_err);

> > +        error_free(child_unrealized_err);

> > +        return;

> > +    } else if (child_unrealized_err != NULL) {

> > +        error_propagate(errp, child_unrealized_err);

> 

> Then this if-else is not needed either.

> 

Yes.

> Otherwise,

> 

> Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

> 

Thanks. Will fix it!

Best regards,
-Gonglei

> Regards,

> Peter

> 

> >          return;

> >      }

> >

> > --

> > 1.7.12.4

> >

> >

> >
diff mbox

Patch

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 4a1ac5b..c869520 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -813,6 +813,7 @@  static void device_set_realized(Object *obj, bool value, Error **errp)
     DeviceClass *dc = DEVICE_GET_CLASS(dev);
     BusState *bus;
     Error *local_err = NULL;
+    Error *child_unrealized_err = NULL;
 
     if (dev->hotplugged && !dc->hotpluggable) {
         error_set(errp, QERR_DEVICE_NO_HOTPLUG, object_get_typename(obj));
@@ -875,13 +876,17 @@  static void device_set_realized(Object *obj, bool value, Error **errp)
             object_property_set_bool(OBJECT(bus), false, "realized",
                                      &local_err);
             if (local_err != NULL) {
-                break;
+                if (child_unrealized_err == NULL) {
+                    child_unrealized_err = error_copy(local_err);
+                }
+                error_free(local_err);
+                local_err = NULL;
             }
         }
-        if (qdev_get_vmsd(dev) && local_err == NULL) {
+        if (qdev_get_vmsd(dev)) {
             vmstate_unregister(dev, qdev_get_vmsd(dev), dev);
         }
-        if (dc->unrealize && local_err == NULL) {
+        if (dc->unrealize) {
             dc->unrealize(dev, &local_err);
         }
         dev->pending_deleted_event = true;
@@ -889,6 +894,10 @@  static void device_set_realized(Object *obj, bool value, Error **errp)
 
     if (local_err != NULL) {
         error_propagate(errp, local_err);
+        error_free(child_unrealized_err);
+        return;
+    } else if (child_unrealized_err != NULL) {
+        error_propagate(errp, child_unrealized_err);
         return;
     }