From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:43661) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Sypk1-0002Og-Hf for qemu-devel@nongnu.org; Tue, 07 Aug 2012 15:46:58 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Sypk0-0005wa-1H for qemu-devel@nongnu.org; Tue, 07 Aug 2012 15:46:57 -0400 Received: from e9.ny.us.ibm.com ([32.97.182.139]:48355) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Sypjz-0005vn-U6 for qemu-devel@nongnu.org; Tue, 07 Aug 2012 15:46:55 -0400 Received: from /spool/local by e9.ny.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 7 Aug 2012 15:46:54 -0400 Received: from d01relay02.pok.ibm.com (d01relay02.pok.ibm.com [9.56.227.234]) by d01dlp02.pok.ibm.com (Postfix) with ESMTP id 6CA7C6E8852 for ; Tue, 7 Aug 2012 15:36:20 -0400 (EDT) Received: from d01av04.pok.ibm.com (d01av04.pok.ibm.com [9.56.224.64]) by d01relay02.pok.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id q77JaJwB167144 for ; Tue, 7 Aug 2012 15:36:19 -0400 Received: from d01av04.pok.ibm.com (loopback [127.0.0.1]) by d01av04.pok.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id q77JaDSI019017 for ; Tue, 7 Aug 2012 15:36:19 -0400 Message-ID: <50216E26.9080801@linux.vnet.ibm.com> Date: Tue, 07 Aug 2012 15:36:06 -0400 From: Corey Bryant MIME-Version: 1.0 References: <1344014889-12390-1-git-send-email-coreyb@linux.vnet.ibm.com> <1344014889-12390-4-git-send-email-coreyb@linux.vnet.ibm.com> <20120807173207.GA11051@stefanha-thinkpad.localdomain> In-Reply-To: <20120807173207.GA11051@stefanha-thinkpad.localdomain> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v6 3/6] monitor: Clean up fd sets on monitor disconnect List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: kwolf@redhat.com, aliguori@us.ibm.com, stefanha@linux.vnet.ibm.com, libvir-list@redhat.com, qemu-devel@nongnu.org, lcapitulino@redhat.com, eblake@redhat.com 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. (eblake@redhat.com, >> kwolf@redhat.com, dberrange@redhat.com) >> >> v6: >> -No changes >> >> Signed-off-by: Corey Bryant >> --- >> 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