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

[Markus Armbruster]

Paolo Bonzini <address@hidden> 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 <address@hidden>
> Signed-off-by: Paolo Bonzini <address@hidden>
> ---
>  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 <address@hidden>

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