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

[Kevin Wolf]

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 <address@hidden>
> ---
> 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. (address@hidden,
>   address@hidden, address@hidden)
> 
> v6
>  -Make @fd optional for remove-fd (address@hidden)
>  -Make @fdset-id optional for add-fd (address@hidden)
> 
> v7:
>  -Share fd sets among all monitor connections (address@hidden)
>  -Added mon_refcount to keep track of monitor connection count.
> 
> v8:
>  -Add opaque string to add-fd/query-fdsets.
>   (address@hidden)
>  -Use camel case for structures. (address@hidden)
>  -Don't return in-use and refcount from query-fdsets.
>   (address@hidden)
>  -Don't return removed fd's from query-fdsets.
>   (address@hidden)
>  -Use fdset-id rather than fdset_id. (address@hidden)
>  -Fix fd leak in qmp_add_fd(). (address@hidden)
>  -Update QMP errors. (address@hidden, address@hidden)
> 
>  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?

Kevin