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

[Corey Bryant <coreyb@linux.vnet.ibm.com>]

On 08/09/2012 06:11 AM, Kevin Wolf wrote:
> Am 07.08.2012 18:49, schrieb Eric Blake:
>> 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.
>
> Why do we even include removed=true file descriptors in query-fdsets?
> Shouldn't they appear to be, well, removed from a clients POV?
>
> The problem with adding flags is the same as with errno numbers: How to
> do it in a platform independent way? The management application might
> run on a different OS than qemu, so a numeric 'flags' field could have
> an entirely different meaning there.
>
> We could add bools for some flags and an enum for
> O_RDONLY/O_WRONLY/O_RDWR, but it's probably better to wait until we know
> which of them we really need.

Based on the other email thread that's going on in parallel to this, I 
think these issues are resolved.

-- 
Regards,
Corey