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(a)linux.vnet.ibm.com&gt; --- 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(a)redhat.com, kwolf(a)redhat.com, dberrange(a)redhat.com) v6 -Make @fd optional for remove-fd (eblake(a)redhat.com) -Make @fdset-id optional for add-fd (eblake(a)redhat.com) v7: -Share fd sets among all monitor connections (kwolf(a)redhat.com) -Added mon_refcount to keep track of monitor connection count. v8: -Add opaque string to add-fd/query-fdsets. (stefanha(a)linux.vnet.ibm.com) -Use camel case for structures. (stefanha(a)linux.vnet.ibm.com) -Don't return in-use and refcount from query-fdsets. (stefanha(a)linux.vnet.ibm.com) -Don't return removed fd's from query-fdsets. (stefanha(a)linux.vnet.ibm.com) -Use fdset-id rather than fdset_id. (eblake(a)redhat.com) -Fix fd leak in qmp_add_fd(). (stefanha(a)linux.vnet.ibm.com) -Update QMP errors. (stefanha(a)linux.vnet.ibm.com, eblake(a)redhat.com) v9: -Use fdset-id rather than fdset_id. (eblake(a)redhat.com) -Update example for query-fdsets. (eblake(a)redhat.com) -Close fd immediately on remove-fd. (kwolf(a)redhat.com, eblake(a)redhat.com) -Drop fdset refcount, and check dup_fds instead (in a later patch). (eblake(a)redhat.com) -Move mon_refcount code to a later patch. (kwolf(a)redhat.com) monitor.c | 185 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ qapi-schema.json | 98 +++++++++++++++++++++++++++++ qmp-commands.hx | 122 +++++++++++++++++++++++++++++++++++ 3 files changed, 405 insertions(+) diff --git a/monitor.c b/monitor.c index 49dccfe..c1851f0 100644 --- a/monitor.c +++ b/monitor.c @@ -140,6 +140,23 @@ 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; + QLIST_HEAD(, MonFdsetFd) fds; + QLIST_ENTRY(MonFdset) next; +}; + typedef struct MonitorControl { QObject *id; JSONMessageParser parser; @@ -211,6 +228,7 @@ 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 mon_cmd_t mon_cmds[]; static mon_cmd_t info_cmds[]; @@ -2389,6 +2407,173 @@ 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; + + QLIST_FOREACH_SAFE(mon_fdset_fd, &mon_fdset->fds, next, mon_fdset_fd_next) { + if (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; + + /* 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[60]; + + 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) { + if (mon_fdset_fd->fd != fd) { + continue; + } + mon_fdset_fd->removed = true; + break; + } else { + mon_fdset_fd->removed = true; + } + } + if (has_fd && !mon_fdset_fd) { + goto error; + } + monitor_fdset_cleanup(mon_fdset); + return; + } + +error: + snprintf(fd_str, sizeof(fd_str), + "fdset-id:%" PRId64 ", fd:%" PRId64, fdset_id, 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; + + 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..53cf083 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..f44926c 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -926,6 +926,128 @@ 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": [ + { + "fds": [ + { + "fd": 30, + "opaque": "rdonly:/path/to/file" + }, + { + "fd": 24, + "opaque": "rdwr:/path/to/file" + } + ], + "fdset-id": 1 + }, + { + "fds": [ + { + "fd": 28 + }, + { + "fd": 29 + } + ], + "fdset-id": 0 + }, + ] + } + +Note: The list of fd sets is shared by all monitor connections. + +EQMP + { .name = "block_passwd", .args_type = "device:B,password:s", -- 1.7.10.4

Signed-off-by: Corey Bryant <coreyb(a)linux.vnet.ibm.com&gt; --- v8 -This patch is new in v8. It was reported on a prior fd passing approach and I realized it's needed in this series. (kwolf(a)redhat.com) v9 -No changes block/raw-posix.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/block/raw-posix.c b/block/raw-posix.c index 0dce089..f606211 100644 --- a/block/raw-posix.c +++ b/block/raw-posix.c @@ -1052,8 +1052,10 @@ static int floppy_probe_device(const char *filename) struct floppy_struct fdparam; struct stat st; - if (strstart(filename, "/dev/fd", NULL)) + if (strstart(filename, "/dev/fd", NULL) && + !strstart(filename, "/dev/fdset/", NULL)) { prio = 50; + } fd = open(filename, O_RDONLY | O_NONBLOCK); if (fd < 0) { -- 1.7.10.4

This patch converts all block layer close calls, that correspond to qemu_open calls, to qemu_close. Signed-off-by: Corey Bryant <coreyb(a)linux.vnet.ibm.com&gt; --- v5: -This patch is new in v5. (kwolf(a)redhat.com, eblake(a)redhat.com) v6-v9: -No changes block/raw-posix.c | 24 ++++++++++++------------ block/raw-win32.c | 2 +- block/vmdk.c | 4 ++-- block/vpc.c | 2 +- block/vvfat.c | 12 ++++++------ osdep.c | 5 +++++ qemu-common.h | 1 + savevm.c | 4 ++-- 8 files changed, 30 insertions(+), 24 deletions(-) diff --git a/block/raw-posix.c b/block/raw-posix.c index 08b997e..6be20b1 100644 --- a/block/raw-posix.c +++ b/block/raw-posix.c @@ -271,7 +271,7 @@ static int raw_open_common(BlockDriverState *bs, const char *filename, out_free_buf: qemu_vfree(s->aligned_buf); out_close: - close(fd); + qemu_close(fd); return -errno; } @@ -376,7 +376,7 @@ static void raw_close(BlockDriverState *bs) { BDRVRawState *s = bs->opaque; if (s->fd >= 0) { - close(s->fd); + qemu_close(s->fd); s->fd = -1; if (s->aligned_buf != NULL) qemu_vfree(s->aligned_buf); @@ -580,7 +580,7 @@ static int raw_create(const char *filename, QEMUOptionParameter *options) if (ftruncate(fd, total_size * BDRV_SECTOR_SIZE) != 0) { result = -errno; } - if (close(fd) != 0) { + if (qemu_close(fd) != 0) { result = -errno; } } @@ -850,7 +850,7 @@ static int hdev_open(BlockDriverState *bs, const char *filename, int flags) if (fd < 0) { bsdPath[strlen(bsdPath)-1] = '1'; } else { - close(fd); + qemu_close(fd); } filename = bsdPath; } @@ -889,7 +889,7 @@ static int fd_open(BlockDriverState *bs) last_media_present = (s->fd >= 0); if (s->fd >= 0 && (get_clock() - s->fd_open_time) >= FD_OPEN_TIMEOUT) { - close(s->fd); + qemu_close(s->fd); s->fd = -1; #ifdef DEBUG_FLOPPY printf("Floppy closed\n"); @@ -988,7 +988,7 @@ static int hdev_create(const char *filename, QEMUOptionParameter *options) else if (lseek(fd, 0, SEEK_END) < total_size * BDRV_SECTOR_SIZE) ret = -ENOSPC; - close(fd); + qemu_close(fd); return ret; } @@ -1038,7 +1038,7 @@ static int floppy_open(BlockDriverState *bs, const char *filename, int flags) return ret; /* close fd so that we can reopen it as needed */ - close(s->fd); + qemu_close(s->fd); s->fd = -1; s->fd_media_changed = 1; @@ -1072,7 +1072,7 @@ static int floppy_probe_device(const char *filename) prio = 100; outc: - close(fd); + qemu_close(fd); out: return prio; } @@ -1107,14 +1107,14 @@ static void floppy_eject(BlockDriverState *bs, bool eject_flag) int fd; if (s->fd >= 0) { - close(s->fd); + qemu_close(s->fd); s->fd = -1; } fd = qemu_open(bs->filename, s->open_flags | O_NONBLOCK); if (fd >= 0) { if (ioctl(fd, FDEJECT, 0) < 0) perror("FDEJECT"); - close(fd); + qemu_close(fd); } } @@ -1175,7 +1175,7 @@ static int cdrom_probe_device(const char *filename) prio = 100; outc: - close(fd); + qemu_close(fd); out: return prio; } @@ -1283,7 +1283,7 @@ static int cdrom_reopen(BlockDriverState *bs) * FreeBSD seems to not notice sometimes... */ if (s->fd >= 0) - close(s->fd); + qemu_close(s->fd); fd = qemu_open(bs->filename, s->open_flags, 0644); if (fd < 0) { s->fd = -1; diff --git a/block/raw-win32.c b/block/raw-win32.c index 8d7838d..c56bf83 100644 --- a/block/raw-win32.c +++ b/block/raw-win32.c @@ -261,7 +261,7 @@ static int raw_create(const char *filename, QEMUOptionParameter *options) return -EIO; set_sparse(fd); ftruncate(fd, total_size * 512); - close(fd); + qemu_close(fd); return 0; } diff --git a/block/vmdk.c b/block/vmdk.c index 557dc1b..daee426 100644 --- a/block/vmdk.c +++ b/block/vmdk.c @@ -1258,7 +1258,7 @@ static int vmdk_create_extent(const char *filename, int64_t filesize, ret = 0; exit: - close(fd); + qemu_close(fd); return ret; } @@ -1506,7 +1506,7 @@ static int vmdk_create(const char *filename, QEMUOptionParameter *options) } ret = 0; exit: - close(fd); + qemu_close(fd); return ret; } diff --git a/block/vpc.c b/block/vpc.c index 60ebf5a..c0b82c4 100644 --- a/block/vpc.c +++ b/block/vpc.c @@ -744,7 +744,7 @@ static int vpc_create(const char *filename, QEMUOptionParameter *options) } fail: - close(fd); + qemu_close(fd); return ret; } diff --git a/block/vvfat.c b/block/vvfat.c index 22b586a..59d3c5b 100644 --- a/block/vvfat.c +++ b/block/vvfat.c @@ -1105,7 +1105,7 @@ static inline void vvfat_close_current_file(BDRVVVFATState *s) if(s->current_mapping) { s->current_mapping = NULL; if (s->current_fd) { - close(s->current_fd); + qemu_close(s->current_fd); s->current_fd = 0; } } @@ -2230,7 +2230,7 @@ static int commit_one_file(BDRVVVFATState* s, } if (offset > 0) { if (lseek(fd, offset, SEEK_SET) != offset) { - close(fd); + qemu_close(fd); g_free(cluster); return -3; } @@ -2251,13 +2251,13 @@ static int commit_one_file(BDRVVVFATState* s, (uint8_t*)cluster, (rest_size + 0x1ff) / 0x200); if (ret < 0) { - close(fd); + qemu_close(fd); g_free(cluster); return ret; } if (write(fd, cluster, rest_size) < 0) { - close(fd); + qemu_close(fd); g_free(cluster); return -2; } @@ -2268,11 +2268,11 @@ static int commit_one_file(BDRVVVFATState* s, if (ftruncate(fd, size)) { perror("ftruncate()"); - close(fd); + qemu_close(fd); g_free(cluster); return -4; } - close(fd); + qemu_close(fd); g_free(cluster); return commit_mappings(s, first_cluster, dir_index); diff --git a/osdep.c b/osdep.c index c07faf5..7f876ae 100644 --- a/osdep.c +++ b/osdep.c @@ -107,6 +107,11 @@ int qemu_open(const char *name, int flags, ...) return ret; } +int qemu_close(int fd) +{ + return close(fd); +} + /* * A variant of write(2) which handles partial write. * diff --git a/qemu-common.h b/qemu-common.h index f16079f..e53126d 100644 --- a/qemu-common.h +++ b/qemu-common.h @@ -207,6 +207,7 @@ const char *path(const char *pathname); void *qemu_oom_check(void *ptr); int qemu_open(const char *name, int flags, ...); +int qemu_close(int fd); ssize_t qemu_write_full(int fd, const void *buf, size_t count) QEMU_WARN_UNUSED_RESULT; ssize_t qemu_send_full(int fd, const void *buf, size_t count, int flags) diff --git a/savevm.c b/savevm.c index 6e82b2d..8ecd5d2 100644 --- a/savevm.c +++ b/savevm.c @@ -513,7 +513,7 @@ static void qemu_fill_buffer(QEMUFile *f) * * Returns f->close() return value, or 0 if close function is not set. */ -static int qemu_close(QEMUFile *f) +static int _qemu_fclose(QEMUFile *f) { int ret = 0; if (f->close) { @@ -535,7 +535,7 @@ int qemu_fclose(QEMUFile *f) { int ret; qemu_fflush(f); - ret = qemu_close(f); + ret = _qemu_fclose(f); /* If any error was spotted before closing, we should report it * instead of the close() return value. */ -- 1.7.10.4

On 08/11/2012 09:22 AM, Blue Swirl wrote: Ok I'll change _qemu_fclose() to qemu_fclose_internal(). I'll send out a new version shortly. -- Regards, Corey

On 08/11/2012 07:14 AM, Corey Bryant wrote: Checking the wrong condition: s/fd/dupfd/ This function appears to promise a reasonable errno on failure. However, I don't think monitor_fdset_dup_fd_add guarantees a reasonable errno, and even if it does, close() can corrupt errno. I think that prior to returning here, you either need an explicit errno=ENOMEM, or fix monitor_fdset_dup_fd to guarantee a nice errno, plus a save and restore of errno here. Unless no one cares about errno on failure, in which case your earlier errno=EINVAL can be dropped. -- Eric Blake eblake(a)redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

Fd sets are shared by all monitor connections. Fd sets are considered to be in use while at least one monitor is connected. When the last monitor disconnects, all fds that are members of an fd set with no outstanding dup references are closed. This prevents any fd leakage associated with a client disconnect prior to using a passed fd. Signed-off-by: Corey Bryant <coreyb(a)linux.vnet.ibm.com&gt; --- v5: -This patch is new in v5. -This support addresses concerns from v4 regarding fd leakage if the client disconnects unexpectedly. (eblake(a)redhat.com, kwolf(a)redhat.com, dberrange(a)redhat.com) v6: -No changes v7: -Removed monitor_fdsets_set_in_use() function since we now use mon_refcount to determine if fdsets are in use. -Added monitor_fdsets_cleanup() function, and increment/decrement of mon_refcount when monitor connects/disconnects. v8: -Use camel case for structures. (stefanha(a)linux.vnet.ibm.com) v9: -Define mon_refcount and add corresponding logic to monitor_fdset_cleanup here rather than earlier patch. (kwolf(a)redhat.com) monitor.c | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/monitor.c b/monitor.c index 4f6d2ce..fc7ac9b 100644 --- a/monitor.c +++ b/monitor.c @@ -230,6 +230,7 @@ static inline int mon_print_count_get(const Monitor *mon) { return 0; } 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[]; @@ -2414,7 +2415,8 @@ static void monitor_fdset_cleanup(MonFdset *mon_fdset) MonFdsetFd *mon_fdset_fd_next; QLIST_FOREACH_SAFE(mon_fdset_fd, &mon_fdset->fds, next, mon_fdset_fd_next) { - if (mon_fdset_fd->removed) { + if (mon_fdset_fd->removed || + (QLIST_EMPTY(&mon_fdset->dup_fds) && mon_refcount == 0)) { close(mon_fdset_fd->fd); g_free(mon_fdset_fd->opaque); QLIST_REMOVE(mon_fdset_fd, next); @@ -2428,6 +2430,16 @@ static void monitor_fdset_cleanup(MonFdset *mon_fdset) } } +static void monitor_fdsets_cleanup(void) +{ + MonFdset *mon_fdset; + MonFdset *mon_fdset_next; + + QLIST_FOREACH_SAFE(mon_fdset, &mon_fdsets, next, mon_fdset_next) { + monitor_fdset_cleanup(mon_fdset); + } +} + AddfdInfo *qmp_add_fd(bool has_fdset_id, int64_t fdset_id, bool has_opaque, const char *opaque, Error **errp) { @@ -4859,9 +4871,12 @@ static void monitor_control_event(void *opaque, int event) data = get_qmp_greeting(); monitor_json_emitter(mon, data); qobject_decref(data); + mon_refcount++; break; case CHR_EVENT_CLOSED: json_message_parser_destroy(&mon->mc->parser); + mon_refcount--; + monitor_fdsets_cleanup(); break; } } @@ -4902,6 +4917,12 @@ static void monitor_event(void *opaque, int event) readline_show_prompt(mon->rs); } mon->reset_seen = 1; + mon_refcount++; + break; + + case CHR_EVENT_CLOSED: + mon_refcount--; + monitor_fdsets_cleanup(); break; } } -- 1.7.10.4