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

[Gonglei (Arei)]

> >
> > Forcefully unrealize all children regardless of early-iteration
> > errors(if happened). We should keep going with cleanup operation
> > rather than report a error immediately.
> >
> > Signed-off-by: Gonglei <address@hidden>
> > ---
> >  hw/core/qdev.c | 9 +++------
> >  1 file changed, 3 insertions(+), 6 deletions(-)
> >
> > diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> > index 4a1ac5b..d3ff526 100644
> > --- a/hw/core/qdev.c
> > +++ b/hw/core/qdev.c
> > @@ -873,15 +873,12 @@ static void device_set_realized(Object *obj, bool
> value, Error **errp)
> >      } else if (!value && dev->realized) {
> >          QLIST_FOREACH(bus, &dev->child_bus, sibling) {
> >              object_property_set_bool(OBJECT(bus), false, "realized",
> > -                                     &local_err);
> > -            if (local_err != NULL) {
> > -                break;
> > -            }
> > +                                     NULL);
> 
> Local err should still be set if at least one child fails to
> unrealize. Otherwise the calls gets a silent failure. 

Yes.

> Couple of ways of doing this.
> 
> 1. Propagate only the first failure.
> 2. Raise a generic error to indicate "at least one child failed to unrealize".
> 3. Concatenate all the child unrealize errors together.
> 
> I like option 1 out of simplicity.
> 
OK. Agreed.

> >          }
> > -        if (qdev_get_vmsd(dev) && local_err == NULL) {
> > +        if (qdev_get_vmsd(dev)) {
> 
> You are also forcefully un-vmsding and unrealizing the actual object
> too. You can generalise the command message.
> 
Peter, Do you mean change the commit message to cover this change? 
Or don't un-vmsding and unrealizing actual object when one child 
unrealized failed? Thanks.

Best regards,
-Gonglei

> Regards,
> Peter
> 
> >              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;
> > --
> > 1.7.12.4
> >
> >
> >