From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:38518) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SziE8-0005Fs-Em for qemu-devel@nongnu.org; Fri, 10 Aug 2012 01:57:41 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SziE5-00014a-EX for qemu-devel@nongnu.org; Fri, 10 Aug 2012 01:57:40 -0400 Received: from mx1.redhat.com ([209.132.183.28]:65455) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SziE5-00014V-6O for qemu-devel@nongnu.org; Fri, 10 Aug 2012 01:57:37 -0400 Message-ID: <5024A2C7.6020400@redhat.com> Date: Thu, 09 Aug 2012 23:57:27 -0600 From: Eric Blake 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> In-Reply-To: <1344564649-6272-3-git-send-email-coreyb@linux.vnet.ibm.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="------------enig4BEC4D2EFC8DE95A50A872ED" 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: Corey Bryant 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 This is an OpenPGP/MIME signed message (RFC 2440 and 3156) --------------enig4BEC4D2EFC8DE95A50A872ED Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable 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. >=20 > 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. >=20 > 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 >=20 > Note: These commands are not compatible with the existing getfd > and closefd QMP commands. >=20 > 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 !=3D fdset_id) { > + continue; > + } > + QLIST_FOREACH(mon_fdset_fd, &mon_fdset->fds, next) { > + if (has_fd && mon_fdset_fd->fd !=3D fd) { > + continue; > + } > + mon_fdset_fd->removed =3D 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=3D1,fd=3D3)= , because you aren't checking whether mon_fdset_fd->removed was already tru= e. 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. > +} > + > +FdsetInfoList *qmp_query_fdsets(Error **errp) > +{ > + MonFdset *mon_fdset; > + MonFdsetFd *mon_fdset_fd; > + FdsetInfoList *fdset_list =3D NULL; > + > + QLIST_FOREACH(mon_fdset, &mon_fdsets, next) { > + FdsetInfoList *fdset_info =3D g_malloc0(sizeof(*fdset_info)); > + FdsetFdInfoList *fdsetfd_list =3D NULL; > + > + fdset_info->value =3D g_malloc0(sizeof(*fdset_info->value)); > + fdset_info->value->fdset_id =3D 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. > +++ 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 t= he fd. > +# > +# Returns: @AddfdInfo on success > +# If file descriptor was not received, FdNotSupplied > +# If @fdset_id does not exist, InvalidParameterValue Missed one: s/_/-/ > +## > +# @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/_/-/ > +SQMP > +query-fdsets > +------------- > + > +Return information describing all fd sets. > + > +Arguments: None > + > +Example: > + > +-> { "execute": "query-fdsets" } > +<- { "return": [ > + { > + "fdset-id": 1 missing a comma > + "fds": [ > + { > + "fd": 21, JSON does not permit trailing commas. > + }, > + { > + "fd": 23, and again > + } > + ], > + }, > + { > + "fdset-id": 2 missing a comma > + "fds": [ > + { > + "fd": 22, trailing comma 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. --=20 Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --------------enig4BEC4D2EFC8DE95A50A872ED Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.12 (GNU/Linux) Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/ iQEcBAEBCAAGBQJQJKLHAAoJEKeha0olJ0NqARYH/RbvkalC9lmO6sz0PgvVYAgK 3zk0xj9vkArmARBogVuH0aVpsZNUgftysEFfts3IXaS12I8WexOjUTWCT73ufZCb 07X00YcaG2MXwV7BN0NiqmoUEEZq8tiB66l1ehpUbT4eg5s+dc6GPFS78/oG2IWD 97cSwc2cJIaYejnT03AsPOc3aJljpSwOeuVh6vDyMqNxzNTencBvW/dpMO7f4oQW G1/hXRs7tNGvUHMSCwXFTwnqivz0T1mq65HdQZiOmqaBPYZVLAB+0zNcubKqaAMD 4xzb/8hop6vYTy8G/PfIyr5ZaAKJZFAi3mGdPUPrWGjvWWZWzQCwSrWE5P/+1uw= =+ODP -----END PGP SIGNATURE----- --------------enig4BEC4D2EFC8DE95A50A872ED--