From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:40323) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Sm5yC-0003Uq-4H for qemu-devel@nongnu.org; Tue, 03 Jul 2012 12:29:02 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Sm5y4-0002yN-Ld for qemu-devel@nongnu.org; Tue, 03 Jul 2012 12:28:55 -0400 Received: from e1.ny.us.ibm.com ([32.97.182.141]:42416) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Sm5y4-0002xm-H9 for qemu-devel@nongnu.org; Tue, 03 Jul 2012 12:28:48 -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 ; Tue, 3 Jul 2012 12:28:44 -0400 Received: from d01relay06.pok.ibm.com (d01relay06.pok.ibm.com [9.56.227.116]) by d01dlp01.pok.ibm.com (Postfix) with ESMTP id CE94638C815E for ; Tue, 3 Jul 2012 12:25:36 -0400 (EDT) Received: from d01av02.pok.ibm.com (d01av02.pok.ibm.com [9.56.224.216]) by d01relay06.pok.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id q63GPZG010289592 for ; Tue, 3 Jul 2012 12:25:35 -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 q63GPYnB003571 for ; Tue, 3 Jul 2012 13:25:34 -0300 Message-ID: <4FF31CFD.7030508@linux.vnet.ibm.com> Date: Tue, 03 Jul 2012 12:25:33 -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> In-Reply-To: <4FF316C9.5020100@redhat.com> 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: Kevin Wolf Cc: aliguori@us.ibm.com, stefanha@linux.vnet.ibm.com, libvir-list@redhat.com, qemu-devel@nongnu.org, Luiz Capitulino , pbonzini@redhat.com, Eric Blake On 07/03/2012 11:59 AM, Kevin Wolf wrote: > Am 03.07.2012 17:40, schrieb Corey Bryant: >> Thanks again for taking time to discuss this at today's QEMU community call. >> >> Here's the proposal we discussed at the call. Please let me know if I >> missed anything or if there are any issues with this design. >> >> Proposal Five: New monitor commands enable adding/removing an fd >> to/from a set. The set of fds should all refer to the same file, but >> may have different access flags (ie. O_RDWR, O_RDONLY). qemu_open can >> then dup the fd that has the matching access mode flags. >> PRO: Supports reopen >> PRO: All other commands work without impact by using qemu_open() >> PRO: No fd leakage (fds are associated with monitor connection and, if >> not in use, closed when monitor disconnects) >> PRO: Security-wise this is ok since libvirt can manage the set of fd's >> (ie. it can add/remove an O_RDWR fd to/from the set as needed). >> CON: Not atomic (e.g. doesn't add an fd with single drive_add command). >> USAGE: >> 1. add-fd /dev/fdset/1 FDSET={M} -> qemu adds fd to set named >> "/dev/fdset/1" - command returns qemu fd (e.g fd=4) to caller. libvirt >> in-use flag turned on for fd. > > I thought qemu would rather return the number of the fdset (which it > also assigns if none it passed, i.e. for fdset creation). Does libvirt > need the number of an individual fd? > > If libvirt prefers to assign fdset numbers itself, I'm not against it, > it's just something that wasn't clear to me yet. > That's fine. QEMU can return the fdset number or a string (/dev/fdset/1) if none is specified. And an fdset will need to be specified if adding to an existing set. I think libvirt will need the fd returned by add-fd so that it can evaluate fds returned by query-fd. It's also useful for remove-fd. >> 2. drive_add file=/dev/fdset/1 -> qemu_open uses the first fd from the >> set that has access flags matching the qemu_open action flags. >> qemu_open increments refcount for this fd. >> 3. add-fd /dev/fdset/1 FDSET={M} -> qemu adds fd to set named >> "/dev/fdset/1" - command returns qemu fd to caller (e.g fd=5). libvirt >> in-use flag turned on for fd. >> 3. block-commit -> qemu_open reopens "/dev/fdset/1" by using the first >> fd from the set that has access flags matching the qemu_open action >> flags. qemu_open increments refcount for this fd. >> 4. remove-fd /dev/fdset/1 5 -> caller requests fd==5 be removed from the >> set. turns libvirt in-use flag off marking the fd ready to be closed >> when qemu is done with it. > > If we decided to not return the individual fd numbers to libvirt, file > descriptors would be uniquely identified by an fdset/flags pair here. > Are you saying we'd pass the fdset name and flags parameters on remove-fd to somehow identify the fds to remove? >> 5. qemu_close decrements refcount for fd, and closes fd when refcount is >> zero and libvirt in use flag is off. > > The monitor could just hold another reference, then we save the > additional flag. But that's a qemu implementation detail. > I'm not sure I understand what you mean. >> More functional details: >> -If libvirt crashes it can call "query-fd /dev/fdset/1" to determine >> which fds are open in the set. > > We also need a query-fdsets command that lists all fdsets that exist. If > we add information about single fds to the return value of it, we > probably don't need a separate query-fd that operates on a single fdset. > Yes, good point. And maybe we don't need 2 commands. query-fdsets could return all the sets and all the fds that are in those sets. >> -If monitor connection closes, qemu will close fds that have a refcount >> of zero. Do we also need a qemu in-use flag in case refcount is zero >> and fd is still in use? > > In use by whom? If it's still in use in qemu (as in "in-use flag would > be set") and we have a refcount of zero, then that's a bug. > In use by qemu. I don't think it's a bug. I think there are situations where refcount gets to zero but qemu is still using the fd. >> -This support requires introduction of qemu_close function that will be >> called everywhere in block layer that close is currently called. >> >> Notes: >> -Patch series 1 will include support for all of the above. This will be >> my initial focus. >> -Patch series 2 will include command line support that enables >> association of command line fd with a monitor set. This will be my >> secondary focus, most likely after patch series 1 is applied. > > Thanks, this is a good and as far as I can tell complete summary of what > we discussed. > > Kevin > Definitely! Thank you for all the input. -- Regards, Corey