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

[Peter Xu <peterx@redhat.com>]

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.

> 
> 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,

-- 
Peter Xu