Re: [Qemu-devel] [PATCH] monitor: postpone monitor_qmp_cleanup_queues

[Peter Xu <peterx@redhat.com>]

On Wed, Jun 13, 2018 at 03:30:05PM +0100, Stefan Hajnoczi wrote:
> On Tue, Jun 12, 2018 at 01:47:00PM +0800, Peter Xu wrote:
> > 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 <stefanha@gmail.com> 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 <peterx@redhat.com> writes:
> > > > >> > 
> > > > >> > > Previously we cleanup the queues when we got CLOSED event.  It was used
> > > > >> > 
> > > > >> > we clean up
> > > > >> > 
> > > > >> > > to make sure we won't leftover replies/events of a old client to a new
> > > > >> > 
> > > > >> > we won't send leftover replies/events of an old client
> > > > >> > 
> > > > >> > > client.  Now this patch postpones that until OPENED.
> > > > >> > 
> > > > >> > What if OPENED never comes?
> > > > >> 
> > > > >> 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 approach.
> > > > >
> > > > > I think this patch fixes the problem at the wrong level.  Marc-André's
> > > > > fix seemed like a cleaner solution.
> > > > 
> > > > Is it the right solution?
> > > > 
> > > > I proposed another one:
> > > 
> > > 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:
> > > 
> > > This seems like a chardev bug.  The solution should probably be in the
> > > chardev layer (Marc-André's patch or something else), not in the monitor
> > > (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.
> 
> It's not possible to keep a POLLOUT fd watch since poll(2) would return
> instantly and the event loop would spin.  However, POLLHUP (G_IO_HUP)
> might work.

It seems so.  But I am not confident enough to quickly provide a
mature patch for chardev.  I'll just work around for monitor right
now.  I'll comment with your ideas there and also on the events.  We
can rework upon when we have time with it.

Regards,

-- 
Peter Xu