[Qemu-devel] [PATCH] qdev: free qemu-opts when the QOM path goes away

[Qemu-devel] [PATCH] qdev: free qemu-opts when the QOM path goes away

[Paolo Bonzini]

Otherwise there is a race where the DEVICE_DELETED event has been sent but
attempts to reuse the ID will fail.

Reported-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/core/qdev.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 4ab04aa..92bd8bb 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -1203,7 +1203,6 @@ static void device_finalize(Object *obj)
     NamedGPIOList *ngl, *next;
 
     DeviceState *dev = DEVICE(obj);
-    qemu_opts_del(dev->opts);
 
     QLIST_FOREACH_SAFE(ngl, &dev->gpios, node, next) {
         QLIST_REMOVE(ngl, node);
@@ -1251,6 +1250,9 @@ static void device_unparent(Object *obj)
         qapi_event_send_device_deleted(!!dev->id, dev->id, path, &error_abort);
         g_free(path);
     }
+
+    qemu_opts_del(dev->opts);
+    dev->opts = NULL;
 }
 
 static void device_class_init(ObjectClass *class, void *data)
-- 
2.5.0

Re: [Qemu-devel] [PATCH] qdev: free qemu-opts when the QOM path goes away

[Paolo Bonzini]

On 19/10/2015 13:11, Paolo Bonzini wrote:
> Otherwise there is a race where the DEVICE_DELETED event has been sent but
> attempts to reuse the ID will fail.
> 
> Reported-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Ping?

Paolo

> ---
>  hw/core/qdev.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 4ab04aa..92bd8bb 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -1203,7 +1203,6 @@ static void device_finalize(Object *obj)
>      NamedGPIOList *ngl, *next;
>  
>      DeviceState *dev = DEVICE(obj);
> -    qemu_opts_del(dev->opts);
>  
>      QLIST_FOREACH_SAFE(ngl, &dev->gpios, node, next) {
>          QLIST_REMOVE(ngl, node);
> @@ -1251,6 +1250,9 @@ static void device_unparent(Object *obj)
>          qapi_event_send_device_deleted(!!dev->id, dev->id, path, &error_abort);
>          g_free(path);
>      }
> +
> +    qemu_opts_del(dev->opts);
> +    dev->opts = NULL;
>  }
>  
>  static void device_class_init(ObjectClass *class, void *data)
> 

Re: [Qemu-devel] [PATCH] qdev: free qemu-opts when the QOM path goes away

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Otherwise there is a race where the DEVICE_DELETED event has been sent but
> attempts to reuse the ID will fail.
>
> Reported-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Let's see whether I understand this.

> ---
>  hw/core/qdev.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 4ab04aa..92bd8bb 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -1203,7 +1203,6 @@ static void device_finalize(Object *obj)
>      NamedGPIOList *ngl, *next;
>  
>      DeviceState *dev = DEVICE(obj);
> -    qemu_opts_del(dev->opts);
>  
>      QLIST_FOREACH_SAFE(ngl, &dev->gpios, node, next) {
>          QLIST_REMOVE(ngl, node);
> @@ -1251,6 +1250,9 @@ static void device_unparent(Object *obj)
>          qapi_event_send_device_deleted(!!dev->id, dev->id, path, &error_abort);

DEVICE_DELETED sent here.

>          g_free(path);
>      }
> +
> +    qemu_opts_del(dev->opts);
> +    dev->opts = NULL;
>  }
>  
>  static void device_class_init(ObjectClass *class, void *data)

object_finalize_child_property() runs during unplug:

    static void object_finalize_child_property(Object *obj, const char *name,
                                               void *opaque)
    {
        Object *child = opaque;

        if (child->class->unparent) {
            (child->class->unparent)(child);  <--- calls device_unparent()
        }
        child->parent = NULL;
        object_unref(child);                  <--- calls device_finalize()
    }

device_unparent() sends DEVICE_DELETED, but dev->opts gets only deleted
later, in device_finalize.  If the client tries to reuse the ID in the
meantime, it fails.

Two remarks:

1. Wouldn't it be cleaner to delete dev-opts *before* sending
   DEVICE_DELETED?  Like this:

    +++ b/hw/core/qdev.c
    @@ -1244,6 +1244,9 @@ static void device_unparent(Object *obj)
             dev->parent_bus = NULL;
         }

    +    qemu_opts_del(dev->opts);
    +    dev->opts = NULL;
    +
         /* Only send event if the device had been completely realized */
         if (dev->pending_deleted_event) {
             gchar *path = object_get_canonical_path(OBJECT(dev));

2. If the device is a block device, then unplugging it also deletes its
   backend (ugly wart we keep for backward compatibility; *not* for
   blockdev-add, though).  This backend also has a QemuOpts.  It gets
   deleted in drive_info_del().  Just like device_finalize(), it runs
   within object_unref(), i.e. after DEVICE_DELETED is sent.  Same race,
   different ID, or am I missing something?

   See also https://bugzilla.redhat.com/show_bug.cgi?id=1256044

Re: [Qemu-devel] [PATCH] qdev: free qemu-opts when the QOM path goes away

[Andreas Färber]

Am 04.11.2015 um 19:34 schrieb Markus Armbruster:
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
>> Otherwise there is a race where the DEVICE_DELETED event has been sent but
>> attempts to reuse the ID will fail.
>>
>> Reported-by: Michael S. Tsirkin <mst@redhat.com>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> Let's see whether I understand this.
> 
>> ---
>>  hw/core/qdev.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
>> index 4ab04aa..92bd8bb 100644
>> --- a/hw/core/qdev.c
>> +++ b/hw/core/qdev.c
>> @@ -1203,7 +1203,6 @@ static void device_finalize(Object *obj)
>>      NamedGPIOList *ngl, *next;
>>  
>>      DeviceState *dev = DEVICE(obj);
>> -    qemu_opts_del(dev->opts);
>>  
>>      QLIST_FOREACH_SAFE(ngl, &dev->gpios, node, next) {
>>          QLIST_REMOVE(ngl, node);
>> @@ -1251,6 +1250,9 @@ static void device_unparent(Object *obj)
>>          qapi_event_send_device_deleted(!!dev->id, dev->id, path, &error_abort);
> 
> DEVICE_DELETED sent here.
> 
>>          g_free(path);
>>      }
>> +
>> +    qemu_opts_del(dev->opts);
>> +    dev->opts = NULL;
>>  }
>>  
>>  static void device_class_init(ObjectClass *class, void *data)
> 
> object_finalize_child_property() runs during unplug:
> 
>     static void object_finalize_child_property(Object *obj, const char *name,
>                                                void *opaque)
>     {
>         Object *child = opaque;
> 
>         if (child->class->unparent) {
>             (child->class->unparent)(child);  <--- calls device_unparent()
>         }
>         child->parent = NULL;
>         object_unref(child);                  <--- calls device_finalize()
>     }
> 
> device_unparent() sends DEVICE_DELETED, but dev->opts gets only deleted
> later, in device_finalize.  If the client tries to reuse the ID in the
> meantime, it fails.
> 
> Two remarks:
> 
> 1. Wouldn't it be cleaner to delete dev-opts *before* sending
>    DEVICE_DELETED?  Like this:
> 
>     +++ b/hw/core/qdev.c
>     @@ -1244,6 +1244,9 @@ static void device_unparent(Object *obj)
>              dev->parent_bus = NULL;
>          }
> 
>     +    qemu_opts_del(dev->opts);
>     +    dev->opts = NULL;
>     +
>          /* Only send event if the device had been completely realized */
>          if (dev->pending_deleted_event) {
>              gchar *path = object_get_canonical_path(OBJECT(dev));

To me this proposal sounds sane, but I did not get to tracing the code
flow here. Paolo, which approach do you prefer and why?

> 2. If the device is a block device, then unplugging it also deletes its
>    backend (ugly wart we keep for backward compatibility; *not* for
>    blockdev-add, though).  This backend also has a QemuOpts.  It gets
>    deleted in drive_info_del().  Just like device_finalize(), it runs
>    within object_unref(), i.e. after DEVICE_DELETED is sent.  Same race,
>    different ID, or am I missing something?
> 
>    See also https://bugzilla.redhat.com/show_bug.cgi?id=1256044

If we can leave this patch decoupled from block layer and decide soonish
on the desired approach, I'd be happy to include it in my upcoming
qom-devices pull.

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH] qdev: free qemu-opts when the QOM path goes away

[Paolo Bonzini]

On 05/11/2015 13:06, Andreas Färber wrote:
> > 1. Wouldn't it be cleaner to delete dev-opts *before* sending
> >    DEVICE_DELETED?  Like this:
> > 
> >     +++ b/hw/core/qdev.c
> >     @@ -1244,6 +1244,9 @@ static void device_unparent(Object *obj)
> >              dev->parent_bus = NULL;
> >          }
> > 
> >     +    qemu_opts_del(dev->opts);
> >     +    dev->opts = NULL;
> >     +
> >          /* Only send event if the device had been completely realized */
> >          if (dev->pending_deleted_event) {
> >              gchar *path = object_get_canonical_path(OBJECT(dev));
> 
> To me this proposal sounds sane, but I did not get to tracing the code
> flow here. Paolo, which approach do you prefer and why?

It doesn't really matter, because the BQL is being held here.

On the other hand, if the opts are deleted in finalize, there is an
arbitrary delay because finalize is typically called after a
synchronize_rcu period.

>> > 2. If the device is a block device, then unplugging it also deletes its
>> >    backend (ugly wart we keep for backward compatibility; *not* for
>> >    blockdev-add, though).  This backend also has a QemuOpts.  It gets
>> >    deleted in drive_info_del().  Just like device_finalize(), it runs
>> >    within object_unref(), i.e. after DEVICE_DELETED is sent.  Same race,
>> >    different ID, or am I missing something?
>> > 
>> >    See also https://bugzilla.redhat.com/show_bug.cgi?id=1256044
>
> If we can leave this patch decoupled from block layer and decide soonish
> on the desired approach, I'd be happy to include it in my upcoming
> qom-devices pull.

I agree with you, the block layer bug is separate.

Paolo

Re: [Qemu-devel] [PATCH] qdev: free qemu-opts when the QOM path goes away

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 05/11/2015 13:06, Andreas Färber wrote:
>> > 1. Wouldn't it be cleaner to delete dev-opts *before* sending
>> >    DEVICE_DELETED?  Like this:
>> > 
>> >     +++ b/hw/core/qdev.c
>> >     @@ -1244,6 +1244,9 @@ static void device_unparent(Object *obj)
>> >              dev->parent_bus = NULL;
>> >          }
>> > 
>> >     +    qemu_opts_del(dev->opts);
>> >     +    dev->opts = NULL;
>> >     +
>> >          /* Only send event if the device had been completely realized */
>> >          if (dev->pending_deleted_event) {
>> >              gchar *path = object_get_canonical_path(OBJECT(dev));
>> 
>> To me this proposal sounds sane, but I did not get to tracing the code
>> flow here. Paolo, which approach do you prefer and why?
>
> It doesn't really matter, because the BQL is being held here.
>
> On the other hand, if the opts are deleted in finalize, there is an
> arbitrary delay because finalize is typically called after a
> synchronize_rcu period.
>
>>> > 2. If the device is a block device, then unplugging it also deletes its
>>> >    backend (ugly wart we keep for backward compatibility; *not* for
>>> >    blockdev-add, though).  This backend also has a QemuOpts.  It gets
>>> >    deleted in drive_info_del().  Just like device_finalize(), it runs
>>> >    within object_unref(), i.e. after DEVICE_DELETED is sent.  Same race,
>>> >    different ID, or am I missing something?
>>> > 
>>> >    See also https://bugzilla.redhat.com/show_bug.cgi?id=1256044
>>
>> If we can leave this patch decoupled from block layer and decide soonish
>> on the desired approach, I'd be happy to include it in my upcoming
>> qom-devices pull.
>
> I agree with you, the block layer bug is separate.

Related, but clearly separate.  Mentioning it in the commit message
would be nice, though.

Re: [Qemu-devel] [PATCH] qdev: free qemu-opts when the QOM path goes away

[Andreas Färber]

Am 05.11.2015 um 13:47 schrieb Markus Armbruster:
> Paolo Bonzini <pbonzini@redhat.com> writes:
>> On 05/11/2015 13:06, Andreas Färber wrote:
>>>> 1. Wouldn't it be cleaner to delete dev-opts *before* sending
>>>>    DEVICE_DELETED?  Like this:
>>>>
>>>>     +++ b/hw/core/qdev.c
>>>>     @@ -1244,6 +1244,9 @@ static void device_unparent(Object *obj)
>>>>              dev->parent_bus = NULL;
>>>>          }
>>>>
>>>>     +    qemu_opts_del(dev->opts);
>>>>     +    dev->opts = NULL;
>>>>     +
>>>>          /* Only send event if the device had been completely realized */
>>>>          if (dev->pending_deleted_event) {
>>>>              gchar *path = object_get_canonical_path(OBJECT(dev));
>>>
>>> To me this proposal sounds sane, but I did not get to tracing the code
>>> flow here. Paolo, which approach do you prefer and why?
>>
>> It doesn't really matter, because the BQL is being held here.
>>
>> On the other hand, if the opts are deleted in finalize, there is an
>> arbitrary delay because finalize is typically called after a
>> synchronize_rcu period.
>>
>>>>> 2. If the device is a block device, then unplugging it also deletes its
>>>>>    backend (ugly wart we keep for backward compatibility; *not* for
>>>>>    blockdev-add, though).  This backend also has a QemuOpts.  It gets
>>>>>    deleted in drive_info_del().  Just like device_finalize(), it runs
>>>>>    within object_unref(), i.e. after DEVICE_DELETED is sent.  Same race,
>>>>>    different ID, or am I missing something?
>>>>>
>>>>>    See also https://bugzilla.redhat.com/show_bug.cgi?id=1256044
>>>
>>> If we can leave this patch decoupled from block layer and decide soonish
>>> on the desired approach, I'd be happy to include it in my upcoming
>>> qom-devices pull.
>>
>> I agree with you, the block layer bug is separate.
> 
> Related, but clearly separate.  Mentioning it in the commit message
> would be nice, though.

Paolo, pong: I gathered that I should queue the original patch without
Markus's proposed change, correct? And do you want to add some sentence
to the commit message as requested by Markus?

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH] qdev: free qemu-opts when the QOM path goes away

[Paolo Bonzini]

On 15/01/2016 18:03, Andreas Färber wrote:
> Am 05.11.2015 um 13:47 schrieb Markus Armbruster:
>> Paolo Bonzini <pbonzini@redhat.com> writes:
>>> On 05/11/2015 13:06, Andreas Färber wrote:
>>>>> 1. Wouldn't it be cleaner to delete dev-opts *before* sending
>>>>>    DEVICE_DELETED?  Like this:
>>>>>
>>>>>     +++ b/hw/core/qdev.c
>>>>>     @@ -1244,6 +1244,9 @@ static void device_unparent(Object *obj)
>>>>>              dev->parent_bus = NULL;
>>>>>          }
>>>>>
>>>>>     +    qemu_opts_del(dev->opts);
>>>>>     +    dev->opts = NULL;
>>>>>     +
>>>>>          /* Only send event if the device had been completely realized */
>>>>>          if (dev->pending_deleted_event) {
>>>>>              gchar *path = object_get_canonical_path(OBJECT(dev));
>>>>
>>>> To me this proposal sounds sane, but I did not get to tracing the code
>>>> flow here. Paolo, which approach do you prefer and why?
>>>
>>> It doesn't really matter, because the BQL is being held here.
>>>
>>> On the other hand, if the opts are deleted in finalize, there is an
>>> arbitrary delay because finalize is typically called after a
>>> synchronize_rcu period.
>>>
>>>>>> 2. If the device is a block device, then unplugging it also deletes its
>>>>>>    backend (ugly wart we keep for backward compatibility; *not* for
>>>>>>    blockdev-add, though).  This backend also has a QemuOpts.  It gets
>>>>>>    deleted in drive_info_del().  Just like device_finalize(), it runs
>>>>>>    within object_unref(), i.e. after DEVICE_DELETED is sent.  Same race,
>>>>>>    different ID, or am I missing something?
>>>>>>
>>>>>>    See also https://bugzilla.redhat.com/show_bug.cgi?id=1256044
>>>>
>>>> If we can leave this patch decoupled from block layer and decide soonish
>>>> on the desired approach, I'd be happy to include it in my upcoming
>>>> qom-devices pull.
>>>
>>> I agree with you, the block layer bug is separate.
>>
>> Related, but clearly separate.  Mentioning it in the commit message
>> would be nice, though.
> 
> Paolo, pong: I gathered that I should queue the original patch without
> Markus's proposed change, correct? And do you want to add some sentence
> to the commit message as requested by Markus?

Yes, thanks:

----
Note that similar races exist for other QemuOpts, which this patch
does not attempt to fix.

For example, if the device is a block device, then unplugging it also
deletes its backend.  However, this backend's get deleted in
drive_info_del(), which is only called when properties are
destroyed.  Just like device_finalize(), drive_info_del() is called
some time after DEVICE_DELETED is sent.  A separate patch series has
been sent to plug this other bug.  Character devices also have yet to
be fixed.
-----

Paolo

Re: [Qemu-devel] [PATCH] qdev: free qemu-opts when the QOM path goes away

[Andreas Färber]

Am 15.01.2016 um 18:16 schrieb Paolo Bonzini:
> On 15/01/2016 18:03, Andreas Färber wrote:
>> Am 05.11.2015 um 13:47 schrieb Markus Armbruster:
>>> Paolo Bonzini <pbonzini@redhat.com> writes:
>>>> On 05/11/2015 13:06, Andreas Färber wrote:
>>>>>> 1. Wouldn't it be cleaner to delete dev-opts *before* sending
>>>>>>    DEVICE_DELETED?  Like this:
>>>>>>
>>>>>>     +++ b/hw/core/qdev.c
>>>>>>     @@ -1244,6 +1244,9 @@ static void device_unparent(Object *obj)
>>>>>>              dev->parent_bus = NULL;
>>>>>>          }
>>>>>>
>>>>>>     +    qemu_opts_del(dev->opts);
>>>>>>     +    dev->opts = NULL;
>>>>>>     +
>>>>>>          /* Only send event if the device had been completely realized */
>>>>>>          if (dev->pending_deleted_event) {
>>>>>>              gchar *path = object_get_canonical_path(OBJECT(dev));
>>>>>
>>>>> To me this proposal sounds sane, but I did not get to tracing the code
>>>>> flow here. Paolo, which approach do you prefer and why?
>>>>
>>>> It doesn't really matter, because the BQL is being held here.
>>>>
>>>> On the other hand, if the opts are deleted in finalize, there is an
>>>> arbitrary delay because finalize is typically called after a
>>>> synchronize_rcu period.
>>>>
>>>>>>> 2. If the device is a block device, then unplugging it also deletes its
>>>>>>>    backend (ugly wart we keep for backward compatibility; *not* for
>>>>>>>    blockdev-add, though).  This backend also has a QemuOpts.  It gets
>>>>>>>    deleted in drive_info_del().  Just like device_finalize(), it runs
>>>>>>>    within object_unref(), i.e. after DEVICE_DELETED is sent.  Same race,
>>>>>>>    different ID, or am I missing something?
>>>>>>>
>>>>>>>    See also https://bugzilla.redhat.com/show_bug.cgi?id=1256044
>>>>>
>>>>> If we can leave this patch decoupled from block layer and decide soonish
>>>>> on the desired approach, I'd be happy to include it in my upcoming
>>>>> qom-devices pull.
>>>>
>>>> I agree with you, the block layer bug is separate.
>>>
>>> Related, but clearly separate.  Mentioning it in the commit message
>>> would be nice, though.
>>
>> Paolo, pong: I gathered that I should queue the original patch without
>> Markus's proposed change, correct? And do you want to add some sentence
>> to the commit message as requested by Markus?
> 
> Yes, thanks:
> 
> ----
> Note that similar races exist for other QemuOpts, which this patch
> does not attempt to fix.
> 
> For example, if the device is a block device, then unplugging it also
> deletes its backend.  However, this backend's get deleted in
> drive_info_del(), which is only called when properties are
> destroyed.  Just like device_finalize(), drive_info_del() is called
> some time after DEVICE_DELETED is sent.  A separate patch series has
> been sent to plug this other bug.  Character devices also have yet to
> be fixed.
> -----

Thanks, queued on qom-next with that addition:
https://github.com/afaerber/qemu-cpu/commits/qom-next

I'll leave Markus and others time until Monday for *-by or comments, but
I really need to get out my pull with Daniel's class properties.

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH] qdev: free qemu-opts when the QOM path goes away

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 15/01/2016 18:03, Andreas Färber wrote:
>> Am 05.11.2015 um 13:47 schrieb Markus Armbruster:
>>> Paolo Bonzini <pbonzini@redhat.com> writes:
>>>> On 05/11/2015 13:06, Andreas Färber wrote:
>>>>>> 1. Wouldn't it be cleaner to delete dev-opts *before* sending
>>>>>>    DEVICE_DELETED?  Like this:
>>>>>>
>>>>>>     +++ b/hw/core/qdev.c
>>>>>>     @@ -1244,6 +1244,9 @@ static void device_unparent(Object *obj)
>>>>>>              dev->parent_bus = NULL;
>>>>>>          }
>>>>>>
>>>>>>     +    qemu_opts_del(dev->opts);
>>>>>>     +    dev->opts = NULL;
>>>>>>     +
>>>>>>          /* Only send event if the device had been completely realized */
>>>>>>          if (dev->pending_deleted_event) {
>>>>>>              gchar *path = object_get_canonical_path(OBJECT(dev));
>>>>>
>>>>> To me this proposal sounds sane, but I did not get to tracing the code
>>>>> flow here. Paolo, which approach do you prefer and why?
>>>>
>>>> It doesn't really matter, because the BQL is being held here.
>>>>
>>>> On the other hand, if the opts are deleted in finalize, there is an
>>>> arbitrary delay because finalize is typically called after a
>>>> synchronize_rcu period.
>>>>
>>>>>>> 2. If the device is a block device, then unplugging it also deletes its
>>>>>>>    backend (ugly wart we keep for backward compatibility; *not* for
>>>>>>>    blockdev-add, though).  This backend also has a QemuOpts.  It gets
>>>>>>>    deleted in drive_info_del().  Just like device_finalize(), it runs
>>>>>>>    within object_unref(), i.e. after DEVICE_DELETED is sent.  Same race,
>>>>>>>    different ID, or am I missing something?
>>>>>>>
>>>>>>>    See also https://bugzilla.redhat.com/show_bug.cgi?id=1256044
>>>>>
>>>>> If we can leave this patch decoupled from block layer and decide soonish
>>>>> on the desired approach, I'd be happy to include it in my upcoming
>>>>> qom-devices pull.
>>>>
>>>> I agree with you, the block layer bug is separate.
>>>
>>> Related, but clearly separate.  Mentioning it in the commit message
>>> would be nice, though.
>> 
>> Paolo, pong: I gathered that I should queue the original patch without
>> Markus's proposed change, correct? And do you want to add some sentence
>> to the commit message as requested by Markus?
>
> Yes, thanks:
>
> ----
> Note that similar races exist for other QemuOpts, which this patch
> does not attempt to fix.
>
> For example, if the device is a block device, then unplugging it also
> deletes its backend.  However, this backend's get deleted in
> drive_info_del(), which is only called when properties are
> destroyed.  Just like device_finalize(), drive_info_del() is called
> some time after DEVICE_DELETED is sent.  A separate patch series has
> been sent to plug this other bug.  Character devices also have yet to
> be fixed.

With this addition to the commit message:

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

Re: [Qemu-devel] [PATCH] qdev: free qemu-opts when the QOM path goes away

[Paolo Bonzini]

On 05/11/2015 13:06, Andreas Färber wrote:
> Am 04.11.2015 um 19:34 schrieb Markus Armbruster:
>> Paolo Bonzini <pbonzini@redhat.com> writes:
>>
>>> Otherwise there is a race where the DEVICE_DELETED event has been sent but
>>> attempts to reuse the ID will fail.
>>>
>>> Reported-by: Michael S. Tsirkin <mst@redhat.com>
>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>
>> Let's see whether I understand this.
>>
>>> ---
>>>  hw/core/qdev.c | 4 +++-
>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
>>> index 4ab04aa..92bd8bb 100644
>>> --- a/hw/core/qdev.c
>>> +++ b/hw/core/qdev.c
>>> @@ -1203,7 +1203,6 @@ static void device_finalize(Object *obj)
>>>      NamedGPIOList *ngl, *next;
>>>  
>>>      DeviceState *dev = DEVICE(obj);
>>> -    qemu_opts_del(dev->opts);
>>>  
>>>      QLIST_FOREACH_SAFE(ngl, &dev->gpios, node, next) {
>>>          QLIST_REMOVE(ngl, node);
>>> @@ -1251,6 +1250,9 @@ static void device_unparent(Object *obj)
>>>          qapi_event_send_device_deleted(!!dev->id, dev->id, path, &error_abort);
>>
>> DEVICE_DELETED sent here.
>>
>>>          g_free(path);
>>>      }
>>> +
>>> +    qemu_opts_del(dev->opts);
>>> +    dev->opts = NULL;
>>>  }
>>>  
>>>  static void device_class_init(ObjectClass *class, void *data)
>>
>> object_finalize_child_property() runs during unplug:
>>
>>     static void object_finalize_child_property(Object *obj, const char *name,
>>                                                void *opaque)
>>     {
>>         Object *child = opaque;
>>
>>         if (child->class->unparent) {
>>             (child->class->unparent)(child);  <--- calls device_unparent()
>>         }
>>         child->parent = NULL;
>>         object_unref(child);                  <--- calls device_finalize()
>>     }
>>
>> device_unparent() sends DEVICE_DELETED, but dev->opts gets only deleted
>> later, in device_finalize.  If the client tries to reuse the ID in the
>> meantime, it fails.
>>
>> Two remarks:
>>
>> 1. Wouldn't it be cleaner to delete dev-opts *before* sending
>>    DEVICE_DELETED?  Like this:
>>
>>     +++ b/hw/core/qdev.c
>>     @@ -1244,6 +1244,9 @@ static void device_unparent(Object *obj)
>>              dev->parent_bus = NULL;
>>          }
>>
>>     +    qemu_opts_del(dev->opts);
>>     +    dev->opts = NULL;
>>     +
>>          /* Only send event if the device had been completely realized */
>>          if (dev->pending_deleted_event) {
>>              gchar *path = object_get_canonical_path(OBJECT(dev));
> 
> To me this proposal sounds sane, but I did not get to tracing the code
> flow here. Paolo, which approach do you prefer and why?
> 
>> 2. If the device is a block device, then unplugging it also deletes its
>>    backend (ugly wart we keep for backward compatibility; *not* for
>>    blockdev-add, though).  This backend also has a QemuOpts.  It gets
>>    deleted in drive_info_del().  Just like device_finalize(), it runs
>>    within object_unref(), i.e. after DEVICE_DELETED is sent.  Same race,
>>    different ID, or am I missing something?
>>
>>    See also https://bugzilla.redhat.com/show_bug.cgi?id=1256044
> 
> If we can leave this patch decoupled from block layer and decide soonish
> on the desired approach, I'd be happy to include it in my upcoming
> qom-devices pull.

Ping?

Paolo