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

[Corey Bryant <coreyb@linux.vnet.ibm.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 <coreyb@linux.vnet.ibm.com>
>
>> +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