Re: [Qemu-devel] [PATCH v5 2/4] qdev: using NULL instead of local_err for qbus_child unrealize

[Gonglei (Arei)]

> From: address@hidden
> [mailto:address@hidden 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,  <address@hidden> wrote:
> > From: Gonglei <address@hidden>
> >
> > 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 <address@hidden>
> > ---
> >  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 <address@hidden>
> 
Thanks. Will fix it!

Best regards,
-Gonglei

> Regards,
> Peter
> 
> >          return;
> >      }
> >
> > --
> > 1.7.12.4
> >
> >
> >