[Qemu-devel] [PATCH for 2.1] qdev: correctly send DEVICE_DELETED for recursively-deleted devices

[Qemu-devel] [PATCH for 2.1] qdev: correctly send DEVICE_DELETED for recursively-deleted devices

[Paolo Bonzini]

When a device is unparented (i.e. made completely hidden from management)
we want to send a DEVICE_DELETED event only if the device actually was
realized.  This avoids raising DEVICE_DELETED events when device_add
fails.

However, this does not work right for recursively-deleted
devices: the whole tree is _first_ unrealized, _then_ unparented.
Then device_unparent sees realized==false and fails to trigger
the event.  The solution is simply to move have_realized into
the DeviceState struct.  If device_add fails, we never set the
new field to true and DEVICE_DELETED is not sent.

Fixes qemu-iotests testcase 067.

Reported-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/core/qdev.c         | 5 +++--
 include/hw/qdev-core.h | 1 +
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index d1eba3c..c520415 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -848,6 +848,7 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
         if (dev->hotplugged && local_err == NULL) {
             device_reset(dev);
         }
+        dev->pending_deleted_event = false;
     } else if (!value && dev->realized) {
         QLIST_FOREACH(bus, &dev->child_bus, sibling) {
             object_property_set_bool(OBJECT(bus), false, "realized",
@@ -862,6 +863,7 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
         if (dc->unrealize && local_err == NULL) {
             dc->unrealize(dev, &local_err);
         }
+        dev->pending_deleted_event = true;
     }
 
     if (local_err != NULL) {
@@ -972,7 +974,6 @@ static void device_unparent(Object *obj)
 {
     DeviceState *dev = DEVICE(obj);
     BusState *bus;
-    bool have_realized = dev->realized;
 
     if (dev->realized) {
         object_property_set_bool(obj, false, "realized", NULL);
@@ -988,7 +989,7 @@ static void device_unparent(Object *obj)
     }
 
     /* Only send event if the device had been completely realized */
-    if (have_realized) {
+    if (dev->pending_deleted_event) {
         gchar *path = object_get_canonical_path(OBJECT(dev));
 
         qapi_event_send_device_deleted(!!dev->id, dev->id, path, &error_abort);
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 9221cfc..0799ff2 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -156,6 +156,7 @@ struct DeviceState {
 
     const char *id;
     bool realized;
+    bool pending_deleted_event;
     QemuOpts *opts;
     int hotplugged;
     BusState *parent_bus;
-- 
1.9.3

Re: [Qemu-devel] [PATCH for 2.1] qdev: correctly send DEVICE_DELETED for recursively-deleted devices

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> When a device is unparented (i.e. made completely hidden from management)
> we want to send a DEVICE_DELETED event only if the device actually was
> realized.  This avoids raising DEVICE_DELETED events when device_add
> fails.
>
> However, this does not work right for recursively-deleted
> devices: the whole tree is _first_ unrealized, _then_ unparented.
> Then device_unparent sees realized==false and fails to trigger
> the event.  The solution is simply to move have_realized into
> the DeviceState struct.  If device_add fails, we never set the
> new field to true and DEVICE_DELETED is not sent.
>
> Fixes qemu-iotests testcase 067.

Suggest to add "Broken in commit 5942a19" here, to make it clear that
it's a recent regression.

> Reported-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/core/qdev.c         | 5 +++--
>  include/hw/qdev-core.h | 1 +
>  2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index d1eba3c..c520415 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -848,6 +848,7 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
       if (value && !dev->realized) {
[...]
>          if (dev->hotplugged && local_err == NULL) {
>              device_reset(dev);
>          }
> +        dev->pending_deleted_event = false;

Unset on completion of unrealized -> realized transition.

>      } else if (!value && dev->realized) {
>          QLIST_FOREACH(bus, &dev->child_bus, sibling) {
>              object_property_set_bool(OBJECT(bus), false, "realized",
> @@ -862,6 +863,7 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
>          if (dc->unrealize && local_err == NULL) {
>              dc->unrealize(dev, &local_err);
>          }
> +        dev->pending_deleted_event = true;

Set on completion of realized -> unrealized transition.

>      }
>  
>      if (local_err != NULL) {
> @@ -972,7 +974,6 @@ static void device_unparent(Object *obj)
>  {
>      DeviceState *dev = DEVICE(obj);
>      BusState *bus;
> -    bool have_realized = dev->realized;
>  
>      if (dev->realized) {
>          object_property_set_bool(obj, false, "realized", NULL);
> @@ -988,7 +989,7 @@ static void device_unparent(Object *obj)
>      }
>  
>      /* Only send event if the device had been completely realized */
> -    if (have_realized) {
> +    if (dev->pending_deleted_event) {
>          gchar *path = object_get_canonical_path(OBJECT(dev));
>  
>          qapi_event_send_device_deleted(!!dev->id, dev->id, path, &error_abort);

Let's see whether I understand how this works.  Please correct
misunderstandings.

device_unparent() runs right before device deletion, and only then.

First thing it does is setting property "realized" to false.

Does nothing if the device has never been completely realized.
dev->pending_deleted_event remains in its initial state false.
DEVICE_DELETED not sent.  Good.

Else, the device was completely realized at some time.  If it is
currently realized, we get a transition to unrealized right now, setting
dev->pending_deleted_event.  Else, the last transition must have been
realized -> unrealized, setting dev->pending_deleted_event.  Since it
gets unset only on unrealized -> realized, it's still set.

Therefore, dev->pending_deleted_event is set if and only if the device
has been completely realized.

> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index 9221cfc..0799ff2 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -156,6 +156,7 @@ struct DeviceState {
>  
>      const char *id;
>      bool realized;
> +    bool pending_deleted_event;
>      QemuOpts *opts;
>      int hotplugged;
>      BusState *parent_bus;

Reviewed-by: Markus Armbruster <armbru@redhat.com>

(Tested, too, but my r-by subsumes that here)

Re: [Qemu-devel] [PATCH for 2.1] qdev: correctly send DEVICE_DELETED for recursively-deleted devices

[Andreas Färber]

Am 27.06.2014 09:16, schrieb Markus Armbruster:
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
>> When a device is unparented (i.e. made completely hidden from management)
>> we want to send a DEVICE_DELETED event only if the device actually was
>> realized.  This avoids raising DEVICE_DELETED events when device_add
>> fails.
>>
>> However, this does not work right for recursively-deleted
>> devices: the whole tree is _first_ unrealized, _then_ unparented.
>> Then device_unparent sees realized==false and fails to trigger
>> the event.  The solution is simply to move have_realized into
>> the DeviceState struct.  If device_add fails, we never set the
>> new field to true and DEVICE_DELETED is not sent.
>>
>> Fixes qemu-iotests testcase 067.
> 
> Suggest to add "Broken in commit 5942a19" here, to make it clear that
> it's a recent regression.

I vaguely recall that something like this was in Bandan's RFC (that I
assume the above commit forward-ported, the subject would be handy to
mention too), but once again without any explanation why, so I saw no
need to apply that during hardfreeze.

Andreas

>> Reported-by: Markus Armbruster <armbru@redhat.com>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH for 2.1] qdev: correctly send DEVICE_DELETED for recursively-deleted devices

[Bandan Das]

Andreas Färber <afaerber@suse.de> writes:

> Am 27.06.2014 09:16, schrieb Markus Armbruster:
>> Paolo Bonzini <pbonzini@redhat.com> writes:
>> 
>>> When a device is unparented (i.e. made completely hidden from management)
>>> we want to send a DEVICE_DELETED event only if the device actually was
>>> realized.  This avoids raising DEVICE_DELETED events when device_add
>>> fails.
>>>
>>> However, this does not work right for recursively-deleted
>>> devices: the whole tree is _first_ unrealized, _then_ unparented.
>>> Then device_unparent sees realized==false and fails to trigger
>>> the event.  The solution is simply to move have_realized into
>>> the DeviceState struct.  If device_add fails, we never set the
>>> new field to true and DEVICE_DELETED is not sent.
>>>
>>> Fixes qemu-iotests testcase 067.
>> 
>> Suggest to add "Broken in commit 5942a19" here, to make it clear that
>> it's a recent regression.
>
> I vaguely recall that something like this was in Bandan's RFC (that I
> assume the above commit forward-ported, the subject would be handy to
> mention too), but once again without any explanation why, so I saw no
> need to apply that during hardfreeze.

Thanks for pointing it out, yes, I believe that my RFC had this case
covered. (For historical accuracy) Can we please include a link to the
RFC in the commit message ?

Thanks,
Bandan

> Andreas
>
>>> Reported-by: Markus Armbruster <armbru@redhat.com>
>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> Reviewed-by: Markus Armbruster <armbru@redhat.com>