[v2,1/3] qdev: store DeviceState's canonical path to use when unparenting

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>
---
 hw/core/qdev.c         | 16 +++++++++++++---
 include/hw/qdev-core.h |  1 +
 2 files changed, 14 insertions(+), 3 deletions(-)

On 10/09/2017 12:06 PM, 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().
> 

> +++ 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

s/it's/its/ (remember, "it's" is only appropriate if "it is" or "it has"
can be substituted in its place; otherwise you meant the possessive "its")

Quoting Eric Blake (2017-10-09 13:11:28)
> On 10/09/2017 12:06 PM, 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().
> > 
> 
> > +++ 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
> 
> s/it's/its/ (remember, "it's" is only appropriate if "it is" or "it has"
> can be substituted in its place; otherwise you meant the possessive "its")

Thanks, just inattentiveness on my part. Will send a v3 soon if there are
no other comments.

> 
> -- 
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.           +1-919-301-3266
> Virtualization:  qemu.org | libvirt.org
>

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;