Re: [Qemu-devel] [PATCH v6 3/6] monitor: Clean up fd sets on monitor disconnect

[Corey Bryant]

On 08/07/2012 01:32 PM, Stefan Hajnoczi wrote:
> On Fri, Aug 03, 2012 at 01:28:06PM -0400, Corey Bryant wrote:
> > Each fd set has a boolean that keeps track of whether or not the
> > fd set is in use by a monitor connection.  When a monitor
> > disconnects, all fds that are members of an fd set with refcount
> > of zero are closed.  This prevents any fd leakage associated with
> > a client disconnect prior to using a passed fd.
> >
> > v5:
> >   -This patch is new in v5.
> >   -This support addresses concerns from v4 regarding fd leakage
> >    if the client disconnects unexpectedly. (address@hidden,
> >    address@hidden, address@hidden)
> >
> > v6:
> >   -No changes
> >
> > Signed-off-by: Corey Bryant <address@hidden>
> > ---
> >   monitor.c |   15 +++++++++++++++
> >   1 file changed, 15 insertions(+)
>
> The lifecycle of fdsets and fds isn't clear to me.  It seems like just a
> refcount in fdset should handle this without extra fields like in_use.

The lifecycle of fdsets and fds starts with add-fd.

I'll explain the lifecycle end of fdsets and fds below.  To follow along 
with the code, this cleanup occurs in monitor_fdset_cleanup().

Fds are closed and removed from an fdset when there are no more open 
dup() fds (refcount == 0) for the fd set, and there are either no 
monitor connections (!in-use) or the fd has been removed with remove-fd. 
 In other words fds get cleaned up when:

(refcount == 0 && (!in-use || removed))

Let me explain each variable:

(1) refcount is incremented when qemu_open() dup()s an fd from an fd set 
and is decremented when qemu_close() closes a dup()d fd.  We don't want 
to close any fds in an fd set if refcount > 0, because this file could 
be reopened with different access mode flags, which would require dup() 
of another fd from the fdset.

(2) in-use is used to prevent fd leakage if a monitor disconnects and 
abandons fds. If libvirt adds fds and then disconnects without issuing a 
command that references the fds, then refcount will be zero, and in-use 
will be false, and the fds will be closed and removed from the fd set. 
When the monitor connection is restored, the query-fdsets command can be 
used to see what fd sets and fds are available.

(3) If the remove-fd command is issued, the fd is marked for removal. 
It won't be closed until there are no more outstanding dup() references 
on the fd set, for similar reasons to why we don't close the fd in (1).

fdsets simply get cleaned up once all fds from the set have been closed 
and removed.

Hopefully this clears things up a bit more.

Please also take a look at the v7 series that I sent out today.  Fd sets 
are now stored globally, rather than one per Monitor object.  This 
simplifies things a bit.

--
Regards,
Corey