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 12:08 PM, Kevin Wolf wrote:
> Am 10.08.2012 04:10, schrieb Corey Bryant:
>> 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>
>> ---
>> v5:
>>   -This patch is new in v5 and replaces the pass-fd QMP command
>>    from v4.
>>   -By grouping fds in fd sets, we ease managability with an fd
>>    set per file, addressing concerns raised in v4 about handling
>>    "reopens" and preventing fd leakage. (eblake@redhat.com,
>>    kwolf@redhat.com, dberrange@redhat.com)
>>
>> v6
>>   -Make @fd optional for remove-fd (eblake@redhat.com)
>>   -Make @fdset-id optional for add-fd (eblake@redhat.com)
>>
>> v7:
>>   -Share fd sets among all monitor connections (kwolf@redhat.com)
>>   -Added mon_refcount to keep track of monitor connection count.
>>
>> v8:
>>   -Add opaque string to add-fd/query-fdsets.
>>    (stefanha@linux.vnet.ibm.com)
>>   -Use camel case for structures. (stefanha@linux.vnet.ibm.com)
>>   -Don't return in-use and refcount from query-fdsets.
>>    (stefanha@linux.vnet.ibm.com)
>>   -Don't return removed fd's from query-fdsets.
>>    (stefanha@linux.vnet.ibm.com)
>>   -Use fdset-id rather than fdset_id. (eblake@redhat.com)
>>   -Fix fd leak in qmp_add_fd(). (stefanha@linux.vnet.ibm.com)
>>   -Update QMP errors. (stefanha@linux.vnet.ibm.com, eblake@redhat.com)
>>
>>   monitor.c        |  188 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   qapi-schema.json |   98 ++++++++++++++++++++++++++++
>>   qmp-commands.hx  |  117 +++++++++++++++++++++++++++++++++
>>   3 files changed, 403 insertions(+)
>>
>> diff --git a/monitor.c b/monitor.c
>> index 49dccfe..f9a0577 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -140,6 +140,24 @@ struct mon_fd_t {
>>       QLIST_ENTRY(mon_fd_t) next;
>>   };
>>
>> +/* file descriptor associated with a file descriptor set */
>> +typedef struct MonFdsetFd MonFdsetFd;
>> +struct MonFdsetFd {
>> +    int fd;
>> +    bool removed;
>> +    char *opaque;
>> +    QLIST_ENTRY(MonFdsetFd) next;
>> +};
>> +
>> +/* file descriptor set containing fds passed via SCM_RIGHTS */
>> +typedef struct MonFdset MonFdset;
>> +struct MonFdset {
>> +    int64_t id;
>> +    int refcount;
>> +    QLIST_HEAD(, MonFdsetFd) fds;
>> +    QLIST_ENTRY(MonFdset) next;
>> +};
>> +
>>   typedef struct MonitorControl {
>>       QObject *id;
>>       JSONMessageParser parser;
>> @@ -211,6 +229,8 @@ static inline int mon_print_count_get(const Monitor *mon) { return 0; }
>>   #define QMP_ACCEPT_UNKNOWNS 1
>>
>>   static QLIST_HEAD(mon_list, Monitor) mon_list;
>> +static QLIST_HEAD(mon_fdsets, MonFdset) mon_fdsets;
>> +static int mon_refcount;
>
> You introduce mon_refcount in this patch and check it against 0 in one
> place, but it never gets changed until patch 3 is applied. Would it make
> sense to do both in the same patch?

Yes, I'll go ahead and move the mon_refcount code from this patch to 
patch 3.

-- 
Regards,
Corey