From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:60156) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SoIFR-0004F5-95 for qemu-devel@nongnu.org; Mon, 09 Jul 2012 13:59:53 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SoIFP-0007Ue-7Y for qemu-devel@nongnu.org; Mon, 09 Jul 2012 13:59:48 -0400 Received: from e1.ny.us.ibm.com ([32.97.182.141]:53794) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SoIFP-0007Tf-2h for qemu-devel@nongnu.org; Mon, 09 Jul 2012 13:59:47 -0400 Received: from /spool/local by e1.ny.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Mon, 9 Jul 2012 13:59:43 -0400 Received: from d01relay04.pok.ibm.com (d01relay04.pok.ibm.com [9.56.227.236]) by d01dlp01.pok.ibm.com (Postfix) with ESMTP id 41E7938C8065 for ; Mon, 9 Jul 2012 13:59:39 -0400 (EDT) Received: from d01av02.pok.ibm.com (d01av02.pok.ibm.com [9.56.224.216]) by d01relay04.pok.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id q69HxchW291726 for ; Mon, 9 Jul 2012 13:59:38 -0400 Received: from d01av02.pok.ibm.com (loopback [127.0.0.1]) by d01av02.pok.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id q69Hxb0c001651 for ; Mon, 9 Jul 2012 14:59:38 -0300 Message-ID: <4FFB1C09.7040907@linux.vnet.ibm.com> Date: Mon, 09 Jul 2012 13:59:37 -0400 From: Corey Bryant MIME-Version: 1.0 References: <1340390174-7493-1-git-send-email-coreyb@linux.vnet.ibm.com> <20120626091004.GA14451@redhat.com> <4FE9A0F0.2050809@redhat.com> <20120626175045.2c7011b3@doriath.home> <4FEA37A9.10707@linux.vnet.ibm.com> <4FEA3D9C.8080205@redhat.com> <4FF21A67.8010100@linux.vnet.ibm.com> <4FF31265.1000308@linux.vnet.ibm.com> <4FF316C9.5020100@redhat.com> <4FF31CFD.7030508@linux.vnet.ibm.com> <4FF325C8.4060401@redhat.com> <4FF33004.5030909@linux.vnet.ibm.com> <4FF33349.10404@redhat.com> <4FF3381D.40101@linux.vnet.ibm.com> <4FF3FA22.6090400@redhat.com> <4FF5AD90.8000305@linux.vnet.ibm.com> <20120709110510.12214347@doriath.home> <4FFAF334.9000807@linux.vnet.ibm.com> <4FFAFCB8.8020508@redhat.com> <20120709131858.20b8d28b@doriath.home> In-Reply-To: <20120709131858.20b8d28b@doriath.home> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v4 0/7] file descriptor passing using pass-fd List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Luiz Capitulino Cc: Kevin Wolf , aliguori@us.ibm.com, stefanha@linux.vnet.ibm.com, libvir-list@redhat.com, qemu-devel@nongnu.org, pbonzini@redhat.com, Eric Blake On 07/09/2012 12:18 PM, Luiz Capitulino wrote: > On Mon, 09 Jul 2012 17:46:00 +0200 > Kevin Wolf wrote: > >> Am 09.07.2012 17:05, schrieb Corey Bryant: >>> I'm not sure this is an issue with current design. I know things have >>> changed a bit as the email threads evolved, so I'll paste the current >>> design that I am working from. Please let me know if you still see any >>> issues. >>> >>> FD passing: >>> ----------- >>> New monitor commands enable adding/removing an fd to/from a set. New >>> monitor command query-fdsets enables querying of current monitor fdsets. >>> The set of fds should all refer to the same file, with each fd having >>> different access flags (ie. O_RDWR, O_RDONLY). qemu_open can then dup >>> the fd that has the matching access mode flags. >>> >>> Design points: >>> -------------- >>> 1. add-fd >>> -> fd is passed via SCM rights and qemu adds fd to first unused fdset >>> (e.g. /dev/fdset/1) > > The fdset should be specified by the client, like: > > { "execute": "add-fd-set", "arguments": { "set-name": "/dev/fdset/1" } } > We could make the fdset name configurable. Then we wouldn't force clients into using the file=/dev/fdset/1 alias on commands like device_add. The risk with this is that clients need to be careful and use a unique name that doesn't conflict with any existing file names. The way it is currently, if add-fd is not given an fdset name, it will generate a new fdset and add the fd to it. add-fd always returns the fdset (int) and received fd (int) on success. (e.g. fdset=1 corresponds to file=/dev/fdset/1). Then the 2nd time you want to add an fd to this set, you have to specify fdset=1 on add-fd. I'll do whatever you all prefer. I think there are advantages in both approaches, however I'm leaning toward the current approach and forcing use of /dev/fdset/1 to keep all fd set names consistent. >>> -> add-fd monitor function initializes the monitor inuse flag for the >>> fdset to true > > Why do we need the inuse flag? > This helps to prevent fd leakage. Let's say the client adds fd=3 to /dev/fdset/1 and then the QMP monitor disconnects. Since the following evaluates to true when the monitor disconnects, the fd will be closed: (refcount == 0 && (!inuse || remove)) Note: refcount is incremented for the fdset on qemu_open and decremented on qemu_close, and no commands caused it to be incremented from zero in this example. >>> -> add-fd monitor function initializes the remove flag for the fd to false >>> -> add-fd returns fdset number and received fd number (e.g fd=3) to caller >>> >>> 2. drive_add file=/dev/fdset/1 >>> -> qemu_open uses the first fd in fdset1 that has access flags matching >>> the qemu_open action flags and has remove flag set to false >>> -> qemu_open increments refcount for the fdset >>> -> Need to make sure that if a command like 'device-add' fails that >>> refcount is not incremented >>> >>> 3. add-fd fdset=1 >>> -> fd is passed via SCM rights >>> -> add-fd monitor function adds the received fd to the specified fdset >>> (or fails if fdset doesn't exist) >>> -> add-fd monitor function initializes the remove flag for the fd to false >>> -> add-fd returns fdset number and received fd number (e.g fd=4) to caller >>> >>> 4. block-commit >>> -> qemu_open performs "reopen" by using the first fd from the fdset that >>> has access flags matching the qemu_open action flags and has remove flag >>> set to false >>> -> qemu_open increments refcount for the fdset >>> -> Need to make sure that if a command like 'block-commit' fails that >>> refcount is not incremented >>> >>> 5. remove-fd fdset=1 fd=4 >>> -> remove-fd monitor function fails if fdset doesn't exist >>> -> remove-fd monitor function turns on remove flag for fd=4 >> >> What was again the reason why we keep removed fds in the fdset at all? >> >> The removed flag would make sense for a fdset after a hypothetical >> close-fdset call because the fdset needs to be kept around until the >> last user closes it, but I think removed fds can be deleted immediately. > > Agreed. > Please take a look at my recent reply to Kevin about this and let me know if it clears things up. -- Regards, Corey