| Message ID | 1344564649-6272-3-git-send-email-coreyb@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
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. > +} > + > +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. > +++ 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/_/-/ > +## > +# @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/_/-/ > +SQMP > +query-fdsets > +------------- > + > +Return information describing all fd sets. > + > +Arguments: None > + > +Example: > + > +-> { "execute": "query-fdsets" } > +<- { "return": [ > + { > + "fdset-id": 1 missing a comma > + "fds": [ > + { > + "fd": 21, JSON does not permit trailing commas. > + }, > + { > + "fd": 23, and again > + } > + ], > + }, > + { > + "fdset-id": 2 missing a comma > + "fds": [ > + { > + "fd": 22, trailing comma 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.
On Thu, Aug 09, 2012 at 10:10:44PM -0400, Corey Bryant wrote: > +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); fd is optional and may be uninitialized. I think the human-readable string should be: if has_fd: fd_str = '%s:%s' % (fdset_id, fd) else: fd_str = '%s' % fdset_id Otherwise, looks good.
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.
On 08/10/2012 03:20 AM, Stefan Hajnoczi wrote: > On Thu, Aug 09, 2012 at 10:10:44PM -0400, Corey Bryant wrote: >> +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); > > fd is optional and may be uninitialized. I think the human-readable > string should be: > > if has_fd: > fd_str = '%s:%s' % (fdset_id, fd) > else: > fd_str = '%s' % fdset_id > > Otherwise, looks good. > Good point, thanks. I'll fix this.
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? Kevin
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.
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; static mon_cmd_t mon_cmds[]; static mon_cmd_t info_cmds[]; @@ -2389,6 +2409,174 @@ int monitor_get_fd(Monitor *mon, const char *fdname) return -1; } +static void monitor_fdset_cleanup(MonFdset *mon_fdset) +{ + MonFdsetFd *mon_fdset_fd; + MonFdsetFd *mon_fdset_fd_next; + + if (mon_fdset->refcount != 0) { + return; + } + + QLIST_FOREACH_SAFE(mon_fdset_fd, &mon_fdset->fds, next, mon_fdset_fd_next) { + if (mon_refcount == 0 || mon_fdset_fd->removed) { + close(mon_fdset_fd->fd); + g_free(mon_fdset_fd->opaque); + QLIST_REMOVE(mon_fdset_fd, next); + g_free(mon_fdset_fd); + } + } + + if (QLIST_EMPTY(&mon_fdset->fds)) { + QLIST_REMOVE(mon_fdset, next); + g_free(mon_fdset); + } +} + +AddfdInfo *qmp_add_fd(bool has_fdset_id, int64_t fdset_id, bool has_opaque, + const char *opaque, Error **errp) +{ + int fd; + Monitor *mon = cur_mon; + MonFdset *mon_fdset; + MonFdsetFd *mon_fdset_fd; + AddfdInfo *fdinfo; + + fd = qemu_chr_fe_get_msgfd(mon->chr); + if (fd == -1) { + error_set(errp, QERR_FD_NOT_SUPPLIED); + goto error; + } + + if (has_fdset_id) { + QLIST_FOREACH(mon_fdset, &mon_fdsets, next) { + if (mon_fdset->id == fdset_id) { + break; + } + } + if (mon_fdset == NULL) { + error_set(errp, QERR_INVALID_PARAMETER_VALUE, "fdset-id", + "an existing fdset-id or no fdset-id"); + goto error; + } + } else { + int64_t fdset_id_prev = -1; + MonFdset *mon_fdset_cur = QLIST_FIRST(&mon_fdsets); + + /* Use first available fdset ID */ + QLIST_FOREACH(mon_fdset, &mon_fdsets, next) { + mon_fdset_cur = mon_fdset; + if (fdset_id_prev == mon_fdset_cur->id - 1) { + fdset_id_prev = mon_fdset_cur->id; + continue; + } + break; + } + + mon_fdset = g_malloc0(sizeof(*mon_fdset)); + mon_fdset->id = fdset_id_prev + 1; + mon_fdset->refcount = 0; + + /* The fdset list is ordered by fdset ID */ + if (mon_fdset->id == 0) { + QLIST_INSERT_HEAD(&mon_fdsets, mon_fdset, next); + } else if (mon_fdset->id < mon_fdset_cur->id) { + QLIST_INSERT_BEFORE(mon_fdset_cur, mon_fdset, next); + } else { + QLIST_INSERT_AFTER(mon_fdset_cur, mon_fdset, next); + } + } + + mon_fdset_fd = g_malloc0(sizeof(*mon_fdset_fd)); + mon_fdset_fd->fd = fd; + mon_fdset_fd->removed = false; + if (has_opaque) { + mon_fdset_fd->opaque = g_strdup(opaque); + } + QLIST_INSERT_HEAD(&mon_fdset->fds, mon_fdset_fd, next); + + fdinfo = g_malloc0(sizeof(*fdinfo)); + fdinfo->fdset_id = mon_fdset->id; + fdinfo->fd = mon_fdset_fd->fd; + + return fdinfo; + +error: + if (fd != -1) { + close(fd); + } + return NULL; +} + +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); +} + +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; + } + + fdsetfd_info = g_malloc0(sizeof(*fdsetfd_info)); + fdsetfd_info->value = g_malloc0(sizeof(*fdsetfd_info->value)); + fdsetfd_info->value->fd = mon_fdset_fd->fd; + if (mon_fdset_fd->opaque) { + fdsetfd_info->value->has_opaque = true; + fdsetfd_info->value->opaque = g_strdup(mon_fdset_fd->opaque); + } else { + fdsetfd_info->value->has_opaque = false; + } + + fdsetfd_info->next = fdsetfd_list; + fdsetfd_list = fdsetfd_info; + } + + fdset_info->value->fds = fdsetfd_list; + + fdset_info->next = fdset_list; + fdset_list = fdset_info; + } + + return fdset_list; +} + /* mon_cmds and info_cmds would be sorted at runtime */ static mon_cmd_t mon_cmds[] = { #include "hmp-commands.h" diff --git a/qapi-schema.json b/qapi-schema.json index bd9c450..e13dc7f 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -2199,3 +2199,101 @@ # Since: 0.14.0 ## { 'command': 'closefd', 'data': {'fdname': 'str'} } + +# @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 +## +{ 'type': 'AddfdInfo', 'data': {'fdset-id': 'int', 'fd': 'int'} } + +## +# @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 +# +# 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', '*opaque': 'str'}, + '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. +# +# If @fd is not specified, all file descriptors in @fdset-id +# will be removed. +## +{ 'command': 'remove-fd', 'data': {'fdset-id': 'int', '*fd': 'int'} } + +## +# @FdsetFdInfo: +# +# Information about a file descriptor that belongs to an fd set. +# +# @fd: The file descriptor value. +# +# @opaque: #optional A free-form string that can be used to describe the fd. +# +# Since: 1.2.0 +## +{ 'type': 'FdsetFdInfo', + 'data': {'fd': 'int', '*opaque': 'str'} } + +## +# @FdsetInfo: +# +# Information about an fd set. +# +# @fdset-id: The ID of the fd set. +# +# @fds: A list of file descriptors that belong to this fd set. +# +# Since: 1.2.0 +## +{ 'type': 'FdsetInfo', + 'data': {'fdset-id': 'int', 'fds': ['FdsetFdInfo']} } + +## +# @query-fdsets: +# +# Return information describing all fd sets. +# +# Returns: A list of @FdsetInfo +# +# Since: 1.2.0 +# +# Note: The list of fd sets is shared by all monitor connections. +# +## +{ 'command': 'query-fdsets', 'returns': ['FdsetInfo'] } diff --git a/qmp-commands.hx b/qmp-commands.hx index ac46638..2160701 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -926,6 +926,123 @@ Example: EQMP + { + .name = "add-fd", + .args_type = "fdset-id:i?,opaque:s?", + .params = "add-fd fdset-id opaque", + .help = "Add a file descriptor, that was passed via SCM rights, to an fd set", + .mhandler.cmd_new = qmp_marshal_input_add_fd, + }, + +SQMP +add-fd +------- + +Add a file descriptor, that was passed via SCM rights, to an fd set. + +Arguments: + +- "fdset-id": The ID of the fd set to add the file descriptor to. + (json-int, optional) +- "opaque": A free-form string that can be used to describe the fd. + (json-string, optional) + +Return a json-object with the following information: + +- "fdset-id": The ID of the fd set that the fd was added to. (json-int) +- "fd": The file descriptor that was received via SCM rights and added to the + fd set. (json-int) + +Example: + +-> { "execute": "add-fd", "arguments": { "fdset-id": 1 } } +<- { "return": { "fdset-id": 1, "fd": 3 } } + +Notes: + +(1) The list of fd sets is shared by all monitor connections. +(2) If "fdset-id" is not specified, a new fd set will be created. + +EQMP + + { + .name = "remove-fd", + .args_type = "fdset-id:i,fd:i?", + .params = "remove-fd fdset-id fd", + .help = "Remove a file descriptor from an fd set", + .mhandler.cmd_new = qmp_marshal_input_remove_fd, + }, + +SQMP +remove-fd +--------- + +Remove a file descriptor from an fd set. + +Arguments: + +- "fdset-id": The ID of the fd set that the file descriptor belongs to. + (json-int) +- "fd": The file descriptor that is to be removed. (json-int, optional) + +Example: + +-> { "execute": "remove-fd", "arguments": { "fdset-id": 1, "fd": 3 } } +<- { "return": {} } + +Notes: + +(1) The list of fd sets is shared by all monitor connections. +(2) If "fd" is not specified, all file descriptors in "fdset-id" will be + removed. + +EQMP + + { + .name = "query-fdsets", + .args_type = "", + .help = "Return information describing all fd sets", + .mhandler.cmd_new = qmp_marshal_input_query_fdsets, + }, + +SQMP +query-fdsets +------------- + +Return information describing all fd sets. + +Arguments: None + +Example: + +-> { "execute": "query-fdsets" } +<- { "return": [ + { + "fdset-id": 1 + "fds": [ + { + "fd": 21, + }, + { + "fd": 23, + } + ], + }, + { + "fdset-id": 2 + "fds": [ + { + "fd": 22, + } + ], + } + ] + } + +Note: The list of fd sets is shared by all monitor connections. + +EQMP + { .name = "block_passwd", .args_type = "device:B,password:s",
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(+)