From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:52408) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Synep-00089v-UH for qemu-devel@nongnu.org; Tue, 07 Aug 2012 13:33:29 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Syneo-0001kZ-DV for qemu-devel@nongnu.org; Tue, 07 Aug 2012 13:33:27 -0400 Received: from e9.ny.us.ibm.com ([32.97.182.139]:56050) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Syneo-0001kS-9J for qemu-devel@nongnu.org; Tue, 07 Aug 2012 13:33:26 -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 13:33:24 -0400 Received: from d01relay03.pok.ibm.com (d01relay03.pok.ibm.com [9.56.227.235]) by d01dlp02.pok.ibm.com (Postfix) with ESMTP id 404826E999A for ; Tue, 7 Aug 2012 13:07:47 -0400 (EDT) Received: from d01av01.pok.ibm.com (d01av01.pok.ibm.com [9.56.224.215]) by d01relay03.pok.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id q77H7jPW226652 for ; Tue, 7 Aug 2012 13:07:46 -0400 Received: from d01av01.pok.ibm.com (loopback [127.0.0.1]) by d01av01.pok.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id q77McZwc026625 for ; Tue, 7 Aug 2012 18:38:37 -0400 Message-ID: <50214B5B.5030008@linux.vnet.ibm.com> Date: Tue, 07 Aug 2012 13:07:39 -0400 From: Corey Bryant MIME-Version: 1.0 References: <1344355108-14786-1-git-send-email-coreyb@linux.vnet.ibm.com> <1344355108-14786-3-git-send-email-coreyb@linux.vnet.ibm.com> <5021472E.3090501@redhat.com> In-Reply-To: <5021472E.3090501@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v7 2/6] qapi: Introduce add-fd, remove-fd, query-fdsets List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: kwolf@redhat.com, aliguori@us.ibm.com, stefanha@linux.vnet.ibm.com, libvir-list@redhat.com, qemu-devel@nongnu.org, lcapitulino@redhat.com On 08/07/2012 12:49 PM, Eric Blake wrote: > On 08/07/2012 09:58 AM, Corey Bryant wrote: >> This patch adds support that enables passing of file descriptors >> to the QEMU monitor where they will be stored in specified file >> descriptor sets. >> >> A file descriptor set can be used by a client like libvirt to >> store file descriptors for the same file. This allows the >> client to open a file with different access modes (O_RDWR, >> O_WRONLY, O_RDONLY) and add/remove the passed fds to/from an fd >> set as needed. This will allow QEMU to (in a later patch in this >> series) "open" and "reopen" the same file by dup()ing the fd in >> the fd set that corresponds to the file, where the fd has the >> matching access mode flag that QEMU requests. >> >> The new QMP commands are: >> add-fd: Add a file descriptor to an fd set >> remove-fd: Remove a file descriptor from an fd set >> query-fdsets: Return information describing all fd sets >> > >> + >> +# @AddfdInfo: >> +# >> +# Information about a file descriptor that was added to an fd set. >> +# >> +# @fdset_id: The ID of the fd set that @fd was added to. >> +# >> +# @fd: The file descriptor that was received via SCM rights and >> +# added to the fd set. >> +# >> +# Since: 1.2.0 > > We're not very consistent on '1.2' vs. '1.2.0' in since listings, but > that's probably worth a global cleanup closer to hard freeze. > I'll make a note of it. Or does Luiz usually do a cleanup? >> +## >> +{ 'type': 'AddfdInfo', 'data': {'fdset_id': 'int', 'fd': 'int'} } > > This is a new command, so s/fdset_id/fdset-id/ > Ok >> + >> +## >> +# @add-fd: >> +# >> +# Add a file descriptor, that was passed via SCM rights, to an fd set. >> +# >> +# @fdset_id: #optional The ID of the fd set to add the file descriptor to. >> +# >> +# Returns: @AddfdInfo on success >> +# If file descriptor was not received, FdNotSupplied >> +# If @fdset_id does not exist, FdSetNotFound >> +# >> +# Notes: The list of fd sets is shared by all monitor connections. >> +# >> +# If @fdset_id is not specified, a new fd set will be created. >> +# >> +# Since: 1.2.0 >> +## >> +{ 'command': 'add-fd', 'data': {'*fdset_id': 'int'}, > > Again, s/fdset_id/fdset-id/ > Ok >> + 'returns': 'AddfdInfo' } >> + >> +## >> +# @remove-fd: >> +# >> +# Remove a file descriptor from an fd set. >> +# >> +# @fdset_id: The ID of the fd set that the file descriptor belongs to. >> +# >> +# @fd: #optional The file descriptor that is to be removed. >> +# >> +# Returns: Nothing on success >> +# If @fdset_id or @fd is not found, FdNotFound >> +# >> +# Since: 1.2.0 >> +# >> +# Notes: The list of fd sets is shared by all monitor connections. >> +# >> +# File descriptors that are removed: >> +# o will not be closed until the reference count corresponding >> +# to @fdset_id reaches zero. >> +# o will not be available for use after successful completion >> +# of the remove-fd command. >> +# >> +# If @fd is not specified, all file descriptors in @fdset_id >> +# will be removed. >> +## >> +{ 'command': 'remove-fd', 'data': {'fdset_id': 'int', '*fd': 'int'} } > > And again. > Ok >> + >> +## >> +# @FdsetFdInfo: >> +# >> +# Information about a file descriptor that belongs to an fd set. >> +# >> +# @fd: The file descriptor value. >> +# >> +# @removed: If true, the remove-fd command has been issued for this fd. >> +# >> +# Since: 1.2.0 >> +## >> +{ 'type': 'FdsetFdInfo', 'data': {'fd': 'int', 'removed': 'bool'} } > > Is it worth providing any additional information? For example, knowing > whether the fd is O_RDONLY, O_WRONLY, or O_RDWR might be beneficial to > management apps trying to discover what fds are already present after a > reconnection, in order to decide whether to close them without having to > resort to /proc/$qemupid/fdinfo/nnn lookups. It might even be worth > marking such information optional, present only when 'removed':false. > It makes sense but I'd like to limit the new functionality at this point so that I can get this support into QEMU 1.2. Can this be added as a follow up patch? >> + >> +## >> +# @FdsetInfo: >> +# >> +# Information about an fd set. >> +# >> +# @fdset_id: The ID of the fd set. >> +# >> +# @refcount: A count of the number of outstanding dup() references to >> +# this fd set. >> +# >> +# @in_use: If true, a monitor is connected and has access to this fd set. >> +# >> +# @fds: A list of file descriptors that belong to this fd set. >> +# >> +# Since: 1.2.0 >> +## >> +{ 'type': 'FdsetInfo', >> + 'data': {'fdset_id': 'int', 'refcount': 'int', 'in_use': 'bool', >> + 'fds': ['FdsetFdInfo']} } > > s/fdset_id/fdset-id/; s/in_use/in-use/ > Ok >> + >> +## >> +# @query-fdsets: >> +# >> +# Return information describing all fd sets. >> +# >> +# Returns: A list of @FdsetInfo >> +# >> +# Since: 1.2.0 >> +# >> +# Notes: The list of fd sets is shared by all monitor connections. >> +# >> +# File descriptors are not closed until @refcount is zero, >> +# and either @in_use is false or @removed is true. >> +# >> +## >> +{ 'command': 'query-fdsets', 'returns': ['FdsetInfo'] } >> diff --git a/qerror.c b/qerror.c >> index 92c4eff..63a0aa1 100644 >> --- a/qerror.c >> +++ b/qerror.c >> @@ -148,6 +148,10 @@ static const QErrorStringTable qerror_table[] = { >> .desc = "No file descriptor supplied via SCM_RIGHTS", >> }, >> { >> + .error_fmt = QERR_FDSET_NOT_FOUND, >> + .desc = "File descriptor set with ID '%(id)' not found", >> + }, > > Conflicts with Luiz's error cleanups. I'm not sure whether libvirt > needs to know this specific error, so I'd rather leave it generic for now. > Ok. I'll try to use something more generic. >> +++ b/qmp-commands.hx >> @@ -926,6 +926,137 @@ Example: >> >> EQMP >> >> + { >> + .name = "add-fd", >> + .args_type = "fdset_id:i?", >> + .params = "add-fd fdset_id", > > More fallout from s/_/-/ in the QMP naming, throughout this file. > Ok -- Regards, Corey