Re: [Qemu-devel] [PATCH v2 1/3] qdev: store DeviceState's canonical path to use when unparenting

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 4448 bytes --]

On Mon, Oct 09, 2017 at 12:06:05PM -0500, Michael Roth wrote:
> device_unparent(dev, ...) is called when a device is unparented,
> either directly, or as a result of a parent device being
> finalized, and handles some final cleanup for the device. Part
> of this includes emiting a DEVICE_DELETED QMP event to notify
> management, which includes the device's path in the composition
> tree as provided by object_get_canonical_path().
> 
> object_get_canonical_path() assumes the device is still connected
> to the machine/root container, and will assert otherwise, but
> in some situations this isn't the case:
> 
> If the parent is finalized as a result of object_unparent(), it
> will still be attached to the composition tree at the time any
> children are unparented as a result of that same call to
> object_unparent(). However, in some cases, object_unparent()
> will complete without finalizing the parent device, due to
> lingering references that won't be released till some time later.
> One such example is if the parent has MemoryRegion children (which
> take a ref on their parent), who in turn have AddressSpace's (which
> take a ref on their regions), since those AddressSpaces get cleaned
> up asynchronously by the RCU thread.
> 
> In this case qdev:device_unparent() may be called for a child Device
> that no longer has a path to the root/machine container, causing
> object_get_canonical_path() to assert.
> 
> Fix this by storing the canonical path during realize() so the
> information will still be available for device_unparent() in such
> cases.
> 
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> Signed-off-by: Greg Kurz <groug@kaod.org>
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> Tested-by: Eric Auger <eric.auger@redhat.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

With the exception of the trivial comment error that's already been
mentioned.

> ---
>  hw/core/qdev.c         | 16 +++++++++++++---
>  include/hw/qdev-core.h |  1 +
>  2 files changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 606ab53c42..0542e1879f 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -928,6 +928,13 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
>              goto post_realize_fail;
>          }
>  
> +        /*
> +         * always free/re-initialize here since the value cannot be cleaned up
> +         * in device_unrealize due to it's usage later on in the unplug path
> +         */
> +        g_free(dev->canonical_path);
> +        dev->canonical_path = object_get_canonical_path(OBJECT(dev));
> +
>          if (qdev_get_vmsd(dev)) {
>              if (vmstate_register_with_alias_id(dev, -1, qdev_get_vmsd(dev), dev,
>                                                 dev->instance_id_alias,
> @@ -984,6 +991,7 @@ child_realize_fail:
>      }
>  
>  post_realize_fail:
> +    g_free(dev->canonical_path);
>      if (dc->unrealize) {
>          dc->unrealize(dev, NULL);
>      }
> @@ -1102,10 +1110,12 @@ static void device_unparent(Object *obj)
>  
>      /* Only send event if the device had been completely realized */
>      if (dev->pending_deleted_event) {
> -        gchar *path = object_get_canonical_path(OBJECT(dev));
> +        g_assert(dev->canonical_path);
>  
> -        qapi_event_send_device_deleted(!!dev->id, dev->id, path, &error_abort);
> -        g_free(path);
> +        qapi_event_send_device_deleted(!!dev->id, dev->id, dev->canonical_path,
> +                                       &error_abort);
> +        g_free(dev->canonical_path);
> +        dev->canonical_path = NULL;
>      }
>  
>      qemu_opts_del(dev->opts);
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index 089146197f..0a71bf83f0 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -153,6 +153,7 @@ struct DeviceState {
>      /*< public >*/
>  
>      const char *id;
> +    char *canonical_path;
>      bool realized;
>      bool pending_deleted_event;
>      QemuOpts *opts;

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]