Re: [Qemu-devel] [PATCH v7 2/6] qapi: Introduce add-fd, remove-fd, query-fdsets

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 5832 bytes --]

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.

> +##
> +{ 'type': 'AddfdInfo', 'data': {'fdset_id': 'int', 'fd': 'int'} }

This is a new command, so s/fdset_id/fdset-id/

> +
> +##
> +# @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/

> +  '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.

> +
> +##
> +# @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.

> +
> +##
> +# @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/

> +
> +##
> +# @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.

> +++ 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.

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]