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:04 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.
>
> 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. 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.

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

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