[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] qdev: free qemu-opts when the QOM path goes awa
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH] qdev: free qemu-opts when the QOM path goes away |
Date: |
Mon, 18 Jan 2016 10:45:54 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
Paolo Bonzini <address@hidden> writes:
> On 15/01/2016 18:03, Andreas Färber wrote:
>> Am 05.11.2015 um 13:47 schrieb Markus Armbruster:
>>> Paolo Bonzini <address@hidden> 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 <address@hidden>