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

[Peter Crosthwaite]

On Tue, Sep 2, 2014 at 6:39 PM, Gonglei (Arei) <address@hidden> wrote:
>> >
>> > 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.
>

Just commit msg. Typo by me, s/command/commit.

Regards,
Peter

> 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
>> >
>> >
>> >