On Tue, Sep 2, 2014 at 10:03 PM, <arei.gong...@huawei.com> wrote:
> From: Gonglei <arei.gong...@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.gong...@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.crosthwa...@xilinx.com>
Regards,
Peter
> return;
> }
>
> --
> 1.7.12.4
>
>
>
