On 15/01/2016 18:03, Andreas Färber wrote:
> Am 05.11.2015 um 13:47 schrieb Markus Armbruster:
>> Paolo Bonzini <pbonz...@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
