[Qemu-devel] [PATCH v4 0/4] Refactor device_set_realized to avoid resource leak.

[Qemu-devel] [PATCH v4 0/4] Refactor device_set_realized to avoid resource leak.

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

after committing
 [PATCH v6 0/9] Refactor PCI/SHPC/PCIE hotplug to use a more generic hotplug API

If devcie hotplugging failed, will casuse resource leak.
This patch series include address resouce leak and two other issuses.

v4 -> v3:
 - add patch 2/4.(Thanks for Peter's suggestion)
 - rework patch 3/4 based on patch 2/4.

v3 -> v2:
 - add cleanup logic for set bus/child_bus realized/unrealized failed.
 - change patch 1/3 commit message, add 'Reviewed-by' tag.

v2 -> v1:
 - rewrite patch 1/3, using error_abort instead of local_err.
 - rewrite patch 2/3, add cleanup logic for different error embranchment.
 - rewrite title of patch 3/3, and a syntax fix.

Gonglei (4):
  qdev: using error_abort instead of using local_err
  qdev: using NULL instead of local_err for qbus_child unrealize
  qdev: add cleanup logic in device_set_realized() to avoid resource
    leak
  pcie: don't assert when hotplug a PCIe device with 'function != 0'

 hw/core/qdev.c | 72 ++++++++++++++++++++++++++++++++++++++--------------------
 hw/pci/pcie.c  |  6 ++++-
 2 files changed, 52 insertions(+), 26 deletions(-)

-- 
1.7.12.4

[Qemu-devel] [PATCH v4 1/4] qdev: using error_abort instead of using local_err

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

This error can not happen normally. If it happens indicates
something very wrong, we should abort QEMU. moreover, The
user can only refer to /machine/peripheral, not
/machine/unattached.

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 hw/core/qdev.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index da1ba48..4a1ac5b 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -820,13 +820,13 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
     }
 
     if (value && !dev->realized) {
-        if (!obj->parent && local_err == NULL) {
+        if (!obj->parent) {
             static int unattached_count;
             gchar *name = g_strdup_printf("device[%d]", unattached_count++);
 
             object_property_add_child(container_get(qdev_get_machine(),
                                                     "/unattached"),
-                                      name, obj, &local_err);
+                                      name, obj, &error_abort);
             g_free(name);
         }
 
-- 
1.7.12.4

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

[arei.gonglei]

From: Gonglei <arei.gonglei@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.

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 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);
         }
-        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) {
             dc->unrealize(dev, &local_err);
         }
         dev->pending_deleted_event = true;
-- 
1.7.12.4

[Qemu-devel] [PATCH v4 3/4] qdev: add cleanup logic in device_set_realized() to avoid resource leak

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

At present, this function doesn't have partial cleanup implemented,
which will cause resource leak in some scenarios.

Example:

1. Assuming that "dc->realize(dev, &local_err)" execute successful
   and local_err == NULL;
2. Executing device hotplug in hotplug_handler_plug(), but failed
  (It is prone to occur). Then local_err != NULL;
3. error_propagate(errp, local_err) and return. But the resources
 which been allocated in dc->realize() will be leaked.
 Simple backtrace:
  dc->realize()
   |->device_realize
            |->pci_qdev_init()
                |->do_pci_register_device()
                |->etc.

Adding fuller cleanup logic which assure that function can
goto appropriate error label as local_err population is
detected as each relevant point.

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 hw/core/qdev.c | 59 +++++++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 42 insertions(+), 17 deletions(-)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index d3ff526..a1fa3a4 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -834,12 +834,14 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
             dc->realize(dev, &local_err);
         }
 
-        if (dev->parent_bus && dev->parent_bus->hotplug_handler &&
-            local_err == NULL) {
+        if (local_err != NULL) {
+            goto fail;
+        }
+
+        if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
             hotplug_handler_plug(dev->parent_bus->hotplug_handler,
                                  dev, &local_err);
-        } else if (local_err == NULL &&
-                   object_dynamic_cast(qdev_get_machine(), TYPE_MACHINE)) {
+        } else if (object_dynamic_cast(qdev_get_machine(), TYPE_MACHINE)) {
             HotplugHandler *hotplug_ctrl;
             MachineState *machine = MACHINE(qdev_get_machine());
             MachineClass *mc = MACHINE_GET_CLASS(machine);
@@ -852,21 +854,25 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
             }
         }
 
-        if (qdev_get_vmsd(dev) && local_err == NULL) {
+        if (local_err != NULL) {
+            goto post_realize_fail;
+        }
+
+        if (qdev_get_vmsd(dev)) {
             vmstate_register_with_alias_id(dev, -1, qdev_get_vmsd(dev), dev,
                                            dev->instance_id_alias,
                                            dev->alias_required_for_version);
         }
-        if (local_err == NULL) {
-            QLIST_FOREACH(bus, &dev->child_bus, sibling) {
-                object_property_set_bool(OBJECT(bus), true, "realized",
-                                         &local_err);
-                if (local_err != NULL) {
-                    break;
-                }
+
+        QLIST_FOREACH(bus, &dev->child_bus, sibling) {
+            object_property_set_bool(OBJECT(bus), true, "realized",
+                                     &local_err);
+            if (local_err != NULL) {
+                goto child_realize_fail;
             }
         }
-        if (dev->hotplugged && local_err == NULL) {
+
+        if (dev->hotplugged) {
             device_reset(dev);
         }
         dev->pending_deleted_event = false;
@@ -882,14 +888,33 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
             dc->unrealize(dev, &local_err);
         }
         dev->pending_deleted_event = true;
-    }
 
-    if (local_err != NULL) {
-        error_propagate(errp, local_err);
-        return;
+        if (local_err != NULL) {
+            goto fail;
+        }
     }
 
     dev->realized = value;
+    return;
+
+child_realize_fail:
+    QLIST_FOREACH(bus, &dev->child_bus, sibling) {
+        object_property_set_bool(OBJECT(bus), false, "realized",
+                                 NULL);
+    }
+
+    if (qdev_get_vmsd(dev)) {
+        vmstate_unregister(dev, qdev_get_vmsd(dev), dev);
+    }
+
+post_realize_fail:
+    if (dc->unrealize) {
+        dc->unrealize(dev, NULL);
+    }
+
+fail:
+    error_propagate(errp, local_err);
+    return;
 }
 
 static bool device_get_hotpluggable(Object *obj, Error **errp)
-- 
1.7.12.4

[Qemu-devel] [PATCH v4 4/4] pcie: don't assert when hotplug a PCIe device with 'function != 0'

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

It's enough to report an error. Assert() is not acceptable
because the error is not a fatal error.

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 hw/pci/pcie.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index 1babddf..ab7f8a2 100644
--- a/hw/pci/pcie.c
+++ b/hw/pci/pcie.c
@@ -254,7 +254,11 @@ void pcie_cap_slot_hotplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
      * Right now, only a device of function = 0 is allowed to be
      * hot plugged/unplugged.
      */
-    assert(PCI_FUNC(pci_dev->devfn) == 0);
+    if (PCI_FUNC(pci_dev->devfn) != 0) {
+        error_setg(errp, "Unsupported device function %d for PCIe hotplugging, "
+                   "only supported function 0", PCI_FUNC(pci_dev->devfn));
+        return;
+    }
 
     pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTSTA,
                                PCI_EXP_SLTSTA_PDS);
-- 
1.7.12.4

Re: [Qemu-devel] [PATCH v4 1/4] qdev: using error_abort instead of using local_err

[Gonglei (Arei)]

> From: Gonglei (Arei)
> Sent: Tuesday, September 02, 2014 4:04 PM
> Subject: [PATCH v4 1/4] qdev: using error_abort instead of using local_err
> Importance: High
> 
> From: Gonglei <arei.gonglei@huawei.com>
> 
> This error can not happen normally. If it happens indicates
> something very wrong, we should abort QEMU. moreover, The
> user can only refer to /machine/peripheral, not
> /machine/unattached.
> 
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> ---

Oops, I forget to add 'Reviewed-by' Peter in this patch. 

Next version will fix this. Sorry!

Best regards,
-Gonglei

>  hw/core/qdev.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index da1ba48..4a1ac5b 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -820,13 +820,13 @@ static void device_set_realized(Object *obj, bool
> value, Error **errp)
>      }
> 
>      if (value && !dev->realized) {
> -        if (!obj->parent && local_err == NULL) {
> +        if (!obj->parent) {
>              static int unattached_count;
>              gchar *name = g_strdup_printf("device[%d]",
> unattached_count++);
> 
> 
> object_property_add_child(container_get(qdev_get_machine(),
> 
> "/unattached"),
> -                                      name, obj, &local_err);
> +                                      name, obj, &error_abort);
>              g_free(name);
>          }
> 
> --
> 1.7.12.4
> 

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,  <arei.gonglei@huawei.com> wrote:
> From: Gonglei <arei.gonglei@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.
>
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> ---
>  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
>
>
>

Re: [Qemu-devel] [PATCH v4 1/4] qdev: using error_abort instead of using local_err

[Michael S. Tsirkin]

On Tue, Sep 02, 2014 at 08:19:22AM +0000, Gonglei (Arei) wrote:
> > From: Gonglei (Arei)
> > Sent: Tuesday, September 02, 2014 4:04 PM
> > Subject: [PATCH v4 1/4] qdev: using error_abort instead of using local_err
> > Importance: High
> > 
> > From: Gonglei <arei.gonglei@huawei.com>
> > 
> > This error can not happen normally. If it happens indicates
> > something very wrong, we should abort QEMU. moreover, The
> > user can only refer to /machine/peripheral, not
> > /machine/unattached.
> > 
> > Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> > ---
> 
> Oops, I forget to add 'Reviewed-by' Peter in this patch. 
> 
> Next version will fix this. Sorry!
> 
> Best regards,
> -Gonglei

No need to repost just for this: reply to the
appropriate patch with Reviewed-by in the body.

> >  hw/core/qdev.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> > index da1ba48..4a1ac5b 100644
> > --- a/hw/core/qdev.c
> > +++ b/hw/core/qdev.c
> > @@ -820,13 +820,13 @@ static void device_set_realized(Object *obj, bool
> > value, Error **errp)
> >      }
> > 
> >      if (value && !dev->realized) {
> > -        if (!obj->parent && local_err == NULL) {
> > +        if (!obj->parent) {
> >              static int unattached_count;
> >              gchar *name = g_strdup_printf("device[%d]",
> > unattached_count++);
> > 
> > 
> > object_property_add_child(container_get(qdev_get_machine(),
> > 
> > "/unattached"),
> > -                                      name, obj, &local_err);
> > +                                      name, obj, &error_abort);
> >              g_free(name);
> >          }
> > 
> > --
> > 1.7.12.4
> > 

Re: [Qemu-devel] [PATCH v4 1/4] qdev: using error_abort instead of using local_err

[Gonglei (Arei)]

> > > Subject: [PATCH v4 1/4] qdev: using error_abort instead of using local_err
> > > Importance: High
> > >
> > > From: Gonglei <arei.gonglei@huawei.com>
> > >
> > > This error can not happen normally. If it happens indicates
> > > something very wrong, we should abort QEMU. moreover, The
> > > user can only refer to /machine/peripheral, not
> > > /machine/unattached.
> > >
> > > Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> > > ---
> >
> > Oops, I forget to add 'Reviewed-by' Peter in this patch.
> >
> > Next version will fix this. Sorry!
> >
> > Best regards,
> > -Gonglei
> 
> No need to repost just for this: reply to the
> appropriate patch with Reviewed-by in the body.
> 
OK, understood. Thanks!

Best regards,
-Gonglei

> > >  hw/core/qdev.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> > > index da1ba48..4a1ac5b 100644
> > > --- a/hw/core/qdev.c
> > > +++ b/hw/core/qdev.c
> > > @@ -820,13 +820,13 @@ static void device_set_realized(Object *obj, bool
> > > value, Error **errp)
> > >      }
> > >
> > >      if (value && !dev->realized) {
> > > -        if (!obj->parent && local_err == NULL) {
> > > +        if (!obj->parent) {
> > >              static int unattached_count;
> > >              gchar *name = g_strdup_printf("device[%d]",
> > > unattached_count++);
> > >
> > >
> > > object_property_add_child(container_get(qdev_get_machine(),
> > >
> > > "/unattached"),
> > > -                                      name, obj, &local_err);
> > > +                                      name, obj, &error_abort);
> > >              g_free(name);
> > >          }
> > >
> > > --
> > > 1.7.12.4
> > >

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 <arei.gonglei@huawei.com>
> > ---
> >  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
> >
> >
> >

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) <arei.gonglei@huawei.com> 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 <arei.gonglei@huawei.com>
>> > ---
>> >  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
>> >
>> >
>> >

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

[Gonglei (Arei)]

> From: peter.crosthwaite@petalogix.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.
> >> >
> >> > Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> >> > ---
> >> >  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.
> 
OK. Thanks. Will do.

Best regards,
-Gonglei