From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47310) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1fSc9J-0000qi-Mg for qemu-devel@nongnu.org; Tue, 12 Jun 2018 01:47:23 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1fSc9G-0005Ly-KN for qemu-devel@nongnu.org; Tue, 12 Jun 2018 01:47:21 -0400 Received: from mx3-rdu2.redhat.com ([66.187.233.73]:36454 helo=mx1.redhat.com) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1fSc9G-0005LS-E5 for qemu-devel@nongnu.org; Tue, 12 Jun 2018 01:47:18 -0400 Date: Tue, 12 Jun 2018 13:47:00 +0800 From: Peter Xu Message-ID: <20180612054700.GN7736@xz-mi> References: <20180604080630.10946-1-peterx@redhat.com> <87tvqeyfs2.fsf@dusky.pond.sub.org> <20180608044235.GA7736@xz-mi> <20180608080446.GH29183@stefanha-x1.localdomain> <87a7s5snce.fsf@dusky.pond.sub.org> <20180611164549.GI17756@stefanha-x1.localdomain> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20180611164549.GI17756@stefanha-x1.localdomain> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH] monitor: postpone monitor_qmp_cleanup_queues List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: Markus Armbruster , Stefan Hajnoczi , Kevin Wolf , qemu-devel@nongnu.org, Max Reitz , =?utf-8?Q?Marc-Andr=C3=A9?= Lureau , Paolo Bonzini , John Snow , "Dr . David Alan Gilbert" On Mon, Jun 11, 2018 at 05:45:49PM +0100, Stefan Hajnoczi wrote: > On Fri, Jun 08, 2018 at 10:18:25AM +0200, Markus Armbruster wrote: > > Stefan Hajnoczi writes: > > > On Fri, Jun 08, 2018 at 12:42:35PM +0800, Peter Xu wrote: > > >> On Thu, Jun 07, 2018 at 01:53:01PM +0200, Markus Armbruster wrote: > > >> > Peter Xu writes: > > >> >=20 > > >> > > Previously we cleanup the queues when we got CLOSED event. It= was used > > >> >=20 > > >> > we clean up > > >> >=20 > > >> > > to make sure we won't leftover replies/events of a old client = to a new > > >> >=20 > > >> > we won't send leftover replies/events of an old client > > >> >=20 > > >> > > client. Now this patch postpones that until OPENED. > > >> >=20 > > >> > What if OPENED never comes? > > >>=20 > > >> Then we clean that up until destruction of the monitor. IMHO it's > > >> fine, but I'm not sure whether that's an overall acceptable approa= ch. > > > > > > I think this patch fixes the problem at the wrong level. Marc-Andr= =C3=A9's > > > fix seemed like a cleaner solution. > >=20 > > Is it the right solution? > >=20 > > I proposed another one: >=20 > Sorry, I won't be able to participate in this because I'm behind on > other patches and tasks. Therefore, feel free to disregard but I'll > give my 2 cents: >=20 > This seems like a chardev bug. The solution should probably be in the > chardev layer (Marc-Andr=C3=A9's patch or something else), not in the m= onitor > (this patch). Yes that's why I said I like Marc-Andre's patch. :) I just don't know an reliable way to achieve what we want there. The thing is that we don't really monitor the ioc_out for fd-typed chardevs. We only do that when we call qemu_chr_fe_add_watch() (e.g., in monitor_flush_locked()) when the writting buffer is full. But normally we can't detect any event from the output side, hence no way to deliever a CLOSED event when the output fd is the last fd that is closed. Maybe we can keep that output watch for the whole lifecycle of the chardev? I'm not sure yet. >=20 > Even if there is a monitor change, it's probably necessary to at least > clarify the meaning of the CLOSE event to reduce the chance of similar > bugs in future chardev users. Makes sense. I'll wait for a few more days (I wanted to enable OOB asap since otherwise we'll reach another rc soon again) to see whether Paolo/Marc-Andre/... has any better idea, otherwise I'll post a patch to fix our problem first, then we turn OOB on alongside with the fix. Thanks, --=20 Peter Xu