From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:32981) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Szoqe-0000M6-0h for qemu-devel@nongnu.org; Fri, 10 Aug 2012 09:01:58 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SzoqZ-0004ic-FT for qemu-devel@nongnu.org; Fri, 10 Aug 2012 09:01:51 -0400 Received: from e8.ny.us.ibm.com ([32.97.182.138]:47033) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SzoqZ-0004iU-Ah for qemu-devel@nongnu.org; Fri, 10 Aug 2012 09:01:47 -0400 Received: from /spool/local by e8.ny.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Fri, 10 Aug 2012 09:01:45 -0400 Received: from d01relay05.pok.ibm.com (d01relay05.pok.ibm.com [9.56.227.237]) by d01dlp01.pok.ibm.com (Postfix) with ESMTP id E0F3138C8047 for ; Fri, 10 Aug 2012 09:01:41 -0400 (EDT) Received: from d01av02.pok.ibm.com (d01av02.pok.ibm.com [9.56.224.216]) by d01relay05.pok.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id q7AD1fZe117232 for ; Fri, 10 Aug 2012 09:01:41 -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 q7AD1e1q024946 for ; Fri, 10 Aug 2012 10:01:40 -0300 Message-ID: <5025062F.6000701@linux.vnet.ibm.com> Date: Fri, 10 Aug 2012 09:01:35 -0400 From: Corey Bryant MIME-Version: 1.0 References: <1344564649-6272-1-git-send-email-coreyb@linux.vnet.ibm.com> <1344564649-6272-3-git-send-email-coreyb@linux.vnet.ibm.com> <5024A2C7.6020400@redhat.com> In-Reply-To: <5024A2C7.6020400@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v8 2/7] 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, pbonzini@redhat.com On 08/10/2012 01:57 AM, Eric Blake wrote: > On 08/09/2012 08:10 PM, 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 >> >> Note: These commands are not compatible with the existing getfd >> and closefd QMP commands. >> >> Signed-off-by: Corey Bryant > >> +void qmp_remove_fd(int64_t fdset_id, bool has_fd, int64_t fd, Error **errp) >> +{ >> + MonFdset *mon_fdset; >> + MonFdsetFd *mon_fdset_fd; >> + char fd_str[20]; >> + >> + QLIST_FOREACH(mon_fdset, &mon_fdsets, next) { >> + if (mon_fdset->id != fdset_id) { >> + continue; >> + } >> + QLIST_FOREACH(mon_fdset_fd, &mon_fdset->fds, next) { >> + if (has_fd && mon_fdset_fd->fd != fd) { >> + continue; >> + } >> + mon_fdset_fd->removed = true; >> + if (has_fd) { >> + break; >> + } >> + } >> + monitor_fdset_cleanup(mon_fdset); >> + return; >> + } >> + snprintf(fd_str, sizeof(fd_str), "%" PRId64, fd); >> + error_set(errp, QERR_FD_NOT_FOUND, fd_str); > > This error catches the case of fdset_id not found, but you never raise > an error when has_fd is true but fd is not found within fdset_id. You > also don't raise an error for back-to-back remove-fd(fdset-id=1,fd=3), > because you aren't checking whether mon_fdset_fd->removed was already true. > > I'm not sure which semantic is better, but I _am_ worried that we have > both semantics in teh same function (are we arguing that this command > succeeds when the fd is gone, even if it already was gone? Or are we > arguing that this command diagnoses failure on an attempt to remove > something that doesn't exist). I guess I'm 60-40 towards always issuing > an error on an attempt to remove something that can't be found or is > already removed. > Thanks for catching this. The code used to fall through to the QERR_FD_NOT_FOUND error before I moved the return outside of the mon_fdset_fd loop. Anyway I intended to set QERR_FD_NOT_FOUND when either the fdset or fd is not found. I'll fix that. I hadn't thought of back-to-back remove-fd's requiring an error. I could go either way on that. But since I'll be touching the code again I'll issue an error for that too. >> +} >> + >> +FdsetInfoList *qmp_query_fdsets(Error **errp) >> +{ >> + MonFdset *mon_fdset; >> + MonFdsetFd *mon_fdset_fd; >> + FdsetInfoList *fdset_list = NULL; >> + >> + QLIST_FOREACH(mon_fdset, &mon_fdsets, next) { >> + FdsetInfoList *fdset_info = g_malloc0(sizeof(*fdset_info)); >> + FdsetFdInfoList *fdsetfd_list = NULL; >> + >> + fdset_info->value = g_malloc0(sizeof(*fdset_info->value)); >> + fdset_info->value->fdset_id = mon_fdset->id; >> + >> + QLIST_FOREACH(mon_fdset_fd, &mon_fdset->fds, next) { >> + FdsetFdInfoList *fdsetfd_info; >> + >> + if (mon_fdset_fd->removed) { >> + continue; >> + } > > This means that an fdset with all fds removed will show up as empty in > the output; I guess that's okay, as libvirt could use that to infer that > the dup()d fds are still in use. The alternative is to keep track of > whether we have output any information about an fd within a set before > including the set itself in the overall output. > You're right, an empty set will be shown in this case. I chose the route of less code. :) I'm going to leave this as-is unless there's an objection. >> +++ b/qapi-schema.json > >> +## >> +# @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. >> +# >> +# @opaque: #optional A free-form string that can be used to describe the fd. >> +# >> +# Returns: @AddfdInfo on success >> +# If file descriptor was not received, FdNotSupplied >> +# If @fdset_id does not exist, InvalidParameterValue > > Missed one: s/_/-/ > Gah! Sorry.. and thanks for catching. >> +## >> +# @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 > > and another s/_/-/ > Ok >> +SQMP >> +query-fdsets >> +------------- >> + >> +Return information describing all fd sets. >> + >> +Arguments: None >> + >> +Example: >> + >> +-> { "execute": "query-fdsets" } >> +<- { "return": [ >> + { >> + "fdset-id": 1 > > missing a comma > Ok. Perhaps I should just use a real example rather than editing the old one! >> + "fds": [ >> + { >> + "fd": 21, > > JSON does not permit trailing commas. > Ok >> + }, >> + { >> + "fd": 23, > > and again > Ok >> + } >> + ], >> + }, >> + { >> + "fdset-id": 2 > > missing a comma > Ok >> + "fds": [ >> + { >> + "fd": 22, > > trailing comma > Ok > Also, it might be nice to include something like: > > "opaque":"rdonly:/path/to/file" > > in one of the examples to give a hint to the reader how to use opaque. > No problem I'll do that. -- Regards, Corey