From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53525) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aK8J7-00009j-Mv for qemu-devel@nongnu.org; Fri, 15 Jan 2016 12:37:07 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aK8J2-0003ZV-MI for qemu-devel@nongnu.org; Fri, 15 Jan 2016 12:37:05 -0500 Received: from mx2.suse.de ([195.135.220.15]:41847) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aK8J2-0003ZP-Cc for qemu-devel@nongnu.org; Fri, 15 Jan 2016 12:37:00 -0500 References: <1445253099-16894-1-git-send-email-pbonzini@redhat.com> <87ziytehr2.fsf@blackfin.pond.sub.org> <563B4629.10903@suse.de> <563B49C1.9080305@redhat.com> <87a8qswr33.fsf@blackfin.pond.sub.org> <5699266B.7080209@suse.de> <56992959.9070504@redhat.com> From: =?UTF-8?Q?Andreas_F=c3=a4rber?= Message-ID: <56992E31.6010909@suse.de> Date: Fri, 15 Jan 2016 18:36:49 +0100 MIME-Version: 1.0 In-Reply-To: <56992959.9070504@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH] qdev: free qemu-opts when the QOM path goes away List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: "Michael S. Tsirkin" , qemu-devel@nongnu.org, Markus Armbruster Am 15.01.2016 um 18:16 schrieb Paolo Bonzini: > On 15/01/2016 18:03, Andreas F=C3=A4rber wrote: >> Am 05.11.2015 um 13:47 schrieb Markus Armbruster: >>> Paolo Bonzini writes: >>>> On 05/11/2015 13:06, Andreas F=C3=A4rber 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 =3D NULL; >>>>>> } >>>>>> >>>>>> + qemu_opts_del(dev->opts); >>>>>> + dev->opts =3D NULL; >>>>>> + >>>>>> /* Only send event if the device had been completely real= ized */ >>>>>> if (dev->pending_deleted_event) { >>>>>> gchar *path =3D object_get_canonical_path(OBJECT(dev)= ); >>>>> >>>>> To me this proposal sounds sane, but I did not get to tracing the c= ode >>>>> 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 delet= es its >>>>>>> backend (ugly wart we keep for backward compatibility; *not* f= or >>>>>>> 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. Sam= e race, >>>>>>> different ID, or am I missing something? >>>>>>> >>>>>>> See also https://bugzilla.redhat.com/show_bug.cgi?id=3D1256044 >>>>> >>>>> If we can leave this patch decoupled from block layer and decide so= onish >>>>> 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 sentenc= e >> to the commit message as requested by Markus? >=20 > Yes, thanks: >=20 > ---- > Note that similar races exist for other QemuOpts, which this patch > does not attempt to fix. >=20 > 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 --=20 SUSE Linux GmbH, Maxfeldstr. 5, 90409 N=C3=BCrnberg, Germany GF: Felix Imend=C3=B6rffer, Jane Smithard, Graham Norton; HRB 21284 (AG N= =C3=BCrnberg)