On 07/23/2012 07:08 AM, Corey Bryant wrote: Wrong fd. You aren't changing cloexec on the socket (s->fd), but on the fd that was received via msg (which you don't know at this point in time). Only here do you know what fd you received. I would write it more like: int flags = 0; #ifdef MSG_CMSG_CLOEXEC flags |= MSG_CMSG_CLOEXEC #endif ret = recvmsg(s->fd, &msg, flags); if (ret > 0 && s->is_unix) { unix_process_msgfd(chr, &msg); #ifndef MSG_CMSG_CLOEXEC qemu_set_cloexec(/* fd determined from msg */) #endif } which almost implies that unix_process_msgfd() should be the function that sets cloexec, but without wasting the time doing so if recvmsg already did the job. -- Eric Blake eblake(a)redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

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. 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) Signed-off-by: Corey Bryant <coreyb(a)linux.vnet.ibm.com&gt; --- monitor.c | 144 +++++++++++++++++++++++++++++++++++++++++++++++++++++- qapi-schema.json | 97 ++++++++++++++++++++++++++++++++++++ qmp-commands.hx | 121 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 361 insertions(+), 1 deletion(-) diff --git a/monitor.c b/monitor.c index 09aa3cd..e27dbbe 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 mon_fdset_fd_t mon_fdset_fd_t; +struct mon_fdset_fd_t { + int fd; + bool removed; + QLIST_ENTRY(mon_fdset_fd_t) next; +}; + +/* file descriptor set containing fds passed via SCM_RIGHTS */ +typedef struct mon_fdset_t mon_fdset_t; +struct mon_fdset_t { + int64_t id; + int refcount; + bool in_use; + QLIST_HEAD(, mon_fdset_fd_t) fds; + QLIST_ENTRY(mon_fdset_t) next; +}; + typedef struct MonitorControl { QObject *id; JSONMessageParser parser; @@ -176,7 +194,8 @@ struct Monitor { int print_calls_nr; #endif QError *error; - QLIST_HEAD(,mon_fd_t) fds; + QLIST_HEAD(, mon_fd_t) fds; + QLIST_HEAD(, mon_fdset_t) fdsets; QLIST_ENTRY(Monitor) entry; }; @@ -2396,6 +2415,129 @@ int monitor_get_fd(Monitor *mon, const char *fdname) return -1; } +static void monitor_fdset_cleanup(mon_fdset_t *mon_fdset) +{ + mon_fdset_fd_t *mon_fdset_fd; + mon_fdset_fd_t *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_fdset->in_use || mon_fdset_fd->removed) { + close(mon_fdset_fd->fd); + 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(int64_t fdset_id, Error **errp) +{ + int fd; + Monitor *mon = cur_mon; + mon_fdset_t *mon_fdset; + mon_fdset_fd_t *mon_fdset_fd; + AddfdInfo *fdinfo; + + fd = qemu_chr_fe_get_msgfd(mon->chr); + if (fd == -1) { + qerror_report(QERR_FD_NOT_SUPPLIED); + return NULL; + } + + QLIST_FOREACH(mon_fdset, &mon->fdsets, next) { + if (mon_fdset->id == fdset_id) { + break; + } + } + + if (!mon_fdset) { + mon_fdset = g_malloc0(sizeof(*mon_fdset)); + mon_fdset->id = fdset_id; + mon_fdset->refcount = 0; + mon_fdset->in_use = true; + QLIST_INSERT_HEAD(&mon->fdsets, mon_fdset, next); + } + + mon_fdset_fd = g_malloc0(sizeof(*mon_fdset_fd)); + mon_fdset_fd->fd = fd; + mon_fdset_fd->removed = false; + QLIST_INSERT_HEAD(&mon_fdset->fds, mon_fdset_fd, next); + + fdinfo = g_malloc0(sizeof(*fdinfo)); + fdinfo->fdset_id = fdset_id; + fdinfo->fd = fd; + + return fdinfo; +} + +void qmp_remove_fd(int64_t fdset_id, int64_t fd, Error **errp) +{ + Monitor *mon = cur_mon; + mon_fdset_t *mon_fdset; + mon_fdset_fd_t *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 (mon_fdset_fd->fd != fd) { + continue; + } + mon_fdset_fd->removed = true; + monitor_fdset_cleanup(mon_fdset); + return; + } + } + snprintf(fd_str, sizeof(fd_str), "%ld", fd); + qerror_report(QERR_FD_NOT_FOUND, fd_str); +} + +FdsetInfoList *qmp_query_fdsets(Error **errp) +{ + Monitor *mon = cur_mon; + mon_fdset_t *mon_fdset; + mon_fdset_fd_t *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; + fdset_info->value->refcount = mon_fdset->refcount; + fdset_info->value->in_use = mon_fdset->in_use; + + QLIST_FOREACH(mon_fdset_fd, &mon_fdset->fds, next) { + FdsetFdInfoList *fdsetfd_info = g_malloc0(sizeof(*fdsetfd_info)); + + fdsetfd_info->value = g_malloc0(sizeof(*fdsetfd_info->value)); + fdsetfd_info->value->fd = mon_fdset_fd->fd; + fdsetfd_info->value->removed = mon_fdset_fd->removed; + + 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 a92adb1..c07ff07 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -1905,3 +1905,100 @@ # 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: The ID of the fd set to add the file descriptor to. +# +# Returns: @AddfdInfo on success +# If file descriptor was not received, FdNotSupplied +# +# Since: 1.2.0 +## +{ 'command': 'add-fd', 'data': {'fdset_id': 'int'}, + '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: 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: File descriptors that are removed: +# o will not be closed until the reference count corresponding +# to @fdset_id reaches zero. +# o will not be available for use after successful completion +# of the remove-fd command. +## +{ '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. +# +# @removed: If true, the remove-fd command has been issued for this fd. +# +# Since: 1.2.0 +## +{ 'type': 'FdsetFdInfo', 'data': {'fd': 'int', 'removed': 'bool'} } + +## +# @FdsetInfo: +# +# Information about an fd set. +# +# @fdset_id: The ID of the fd set. +# +# @refcount: A count of the number of outstanding dup() references to +# this fd set. +# +# @in_use: If true, this fd set is in use by a connected QMP monitor. +# +# @fds: A list of file descriptors that belong to this fd set. +# +# Since: 1.2.0 +## +{ 'type': 'FdsetInfo', + 'data': {'fdset_id': 'int', 'refcount': 'int', 'in_use': 'bool', + 'fds': ['FdsetFdInfo']} } + +## +# @query-fdsets: +# +# Return information describing all fd sets. +# +# Returns: A list of @FdsetInfo +# +# Since: 1.2.0 +# +# Notes: File descriptors are not closed until @refcount is zero, +# and either @in_use is false or @removed is true. +## +{ 'command': 'query-fdsets', 'returns': ['FdsetInfo'] } diff --git a/qmp-commands.hx b/qmp-commands.hx index e3cf3c5..d485df4 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -926,6 +926,127 @@ Example: EQMP + { + .name = "add-fd", + .args_type = "fdset_id:i", + .params = "add-fd fdset_id", + .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) + +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 } } + +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) + +Example: + +-> { "execute": "remove-fd", "arguments": { "fdset_id": 1, "fd": 3 } } +<- { "return": {} } + +Notes: + +(1) File descriptors that are removed: + (a) will not be closed until the reference count corresponding to fdset_id + reaches zero. + (b) will not be available for use after successful completion of the + remove-fd command. + +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 + "refcount": 0, + "in_use": true, + "fds": [ + { + "fd": 21, + "removed": false + }, + { + "fd": 23, + "removed": false + } + ], + }, + { + "fdset_id": 2 + "refcount": 0, + "in_use": true, + "fds": [ + { + "fd": 22, + "removed": false + } + ], + } + ] + } + +Notes: + +(1) File descriptors are not closed until refcount is zero, and + either in_use is false or removed is true. + +EQMP + { .name = "block_passwd", .args_type = "device:B,password:s", -- 1.7.10.4

On 07/25/2012 02:16 PM, Eric Blake wrote: Yes, I agree, I think @fdset_id should be optional. Yes, this makes sense. Ok. -- Regards, Corey

This patch converts all block layer open calls to qemu_open. Note that this adds the O_CLOEXEC flag to the changed open paths when the O_CLOEXEC macro is defined. Signed-off-by: Corey Bryant <coreyb(a)linux.vnet.ibm.com&gt; --- v2: -Convert calls to qemu_open instead of file_open (kwolf(a)redhat.com) -Mention introduction of O_CLOEXEC (kwolf(a)redhat.com) v3-v5: -No changes --- block/raw-posix.c | 18 +++++++++--------- block/raw-win32.c | 4 ++-- block/vdi.c | 5 +++-- block/vmdk.c | 21 +++++++++------------ block/vpc.c | 2 +- block/vvfat.c | 4 ++-- 6 files changed, 26 insertions(+), 28 deletions(-) diff --git a/block/raw-posix.c b/block/raw-posix.c index 0dce089..7408a42 100644 --- a/block/raw-posix.c +++ b/block/raw-posix.c @@ -572,8 +572,8 @@ static int raw_create(const char *filename, QEMUOptionParameter *options) options++; } - fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, - 0644); + fd = qemu_open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, + 0644); if (fd < 0) { result = -errno; } else { @@ -846,7 +846,7 @@ static int hdev_open(BlockDriverState *bs, const char *filename, int flags) if ( bsdPath[ 0 ] != '\0' ) { strcat(bsdPath,"s0"); /* some CDs don't have a partition 0 */ - fd = open(bsdPath, O_RDONLY | O_BINARY | O_LARGEFILE); + fd = qemu_open(bsdPath, O_RDONLY | O_BINARY | O_LARGEFILE); if (fd < 0) { bsdPath[strlen(bsdPath)-1] = '1'; } else { @@ -903,7 +903,7 @@ static int fd_open(BlockDriverState *bs) #endif return -EIO; } - s->fd = open(bs->filename, s->open_flags & ~O_NONBLOCK); + s->fd = qemu_open(bs->filename, s->open_flags & ~O_NONBLOCK); if (s->fd < 0) { s->fd_error_time = get_clock(); s->fd_got_error = 1; @@ -977,7 +977,7 @@ static int hdev_create(const char *filename, QEMUOptionParameter *options) options++; } - fd = open(filename, O_WRONLY | O_BINARY); + fd = qemu_open(filename, O_WRONLY | O_BINARY); if (fd < 0) return -errno; @@ -1055,7 +1055,7 @@ static int floppy_probe_device(const char *filename) if (strstart(filename, "/dev/fd", NULL)) prio = 50; - fd = open(filename, O_RDONLY | O_NONBLOCK); + fd = qemu_open(filename, O_RDONLY | O_NONBLOCK); if (fd < 0) { goto out; } @@ -1108,7 +1108,7 @@ static void floppy_eject(BlockDriverState *bs, bool eject_flag) close(s->fd); s->fd = -1; } - fd = open(bs->filename, s->open_flags | O_NONBLOCK); + fd = qemu_open(bs->filename, s->open_flags | O_NONBLOCK); if (fd >= 0) { if (ioctl(fd, FDEJECT, 0) < 0) perror("FDEJECT"); @@ -1158,7 +1158,7 @@ static int cdrom_probe_device(const char *filename) int prio = 0; struct stat st; - fd = open(filename, O_RDONLY | O_NONBLOCK); + fd = qemu_open(filename, O_RDONLY | O_NONBLOCK); if (fd < 0) { goto out; } @@ -1282,7 +1282,7 @@ static int cdrom_reopen(BlockDriverState *bs) */ if (s->fd >= 0) close(s->fd); - fd = open(bs->filename, s->open_flags, 0644); + fd = qemu_open(bs->filename, s->open_flags, 0644); if (fd < 0) { s->fd = -1; return -EIO; diff --git a/block/raw-win32.c b/block/raw-win32.c index e4b0b75..8d7838d 100644 --- a/block/raw-win32.c +++ b/block/raw-win32.c @@ -255,8 +255,8 @@ static int raw_create(const char *filename, QEMUOptionParameter *options) options++; } - fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, - 0644); + fd = qemu_open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, + 0644); if (fd < 0) return -EIO; set_sparse(fd); diff --git a/block/vdi.c b/block/vdi.c index 57325d6..c4f1529 100644 --- a/block/vdi.c +++ b/block/vdi.c @@ -653,8 +653,9 @@ static int vdi_create(const char *filename, QEMUOptionParameter *options) options++; } - fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY | O_LARGEFILE, - 0644); + fd = qemu_open(filename, + O_WRONLY | O_CREAT | O_TRUNC | O_BINARY | O_LARGEFILE, + 0644); if (fd < 0) { return -errno; } diff --git a/block/vmdk.c b/block/vmdk.c index 18e9b4c..557dc1b 100644 --- a/block/vmdk.c +++ b/block/vmdk.c @@ -1161,10 +1161,9 @@ static int vmdk_create_extent(const char *filename, int64_t filesize, VMDK4Header header; uint32_t tmp, magic, grains, gd_size, gt_size, gt_count; - fd = open( - filename, - O_WRONLY | O_CREAT | O_TRUNC | O_BINARY | O_LARGEFILE, - 0644); + fd = qemu_open(filename, + O_WRONLY | O_CREAT | O_TRUNC | O_BINARY | O_LARGEFILE, + 0644); if (fd < 0) { return -errno; } @@ -1484,15 +1483,13 @@ static int vmdk_create(const char *filename, QEMUOptionParameter *options) (flags & BLOCK_FLAG_COMPAT6 ? 6 : 4), total_size / (int64_t)(63 * 16 * 512)); if (split || flat) { - fd = open( - filename, - O_WRONLY | O_CREAT | O_TRUNC | O_BINARY | O_LARGEFILE, - 0644); + fd = qemu_open(filename, + O_WRONLY | O_CREAT | O_TRUNC | O_BINARY | O_LARGEFILE, + 0644); } else { - fd = open( - filename, - O_WRONLY | O_BINARY | O_LARGEFILE, - 0644); + fd = qemu_open(filename, + O_WRONLY | O_BINARY | O_LARGEFILE, + 0644); } if (fd < 0) { return -errno; diff --git a/block/vpc.c b/block/vpc.c index 5cd13d1..60ebf5a 100644 --- a/block/vpc.c +++ b/block/vpc.c @@ -678,7 +678,7 @@ static int vpc_create(const char *filename, QEMUOptionParameter *options) } /* Create the file */ - fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, 0644); + fd = qemu_open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, 0644); if (fd < 0) { return -EIO; } diff --git a/block/vvfat.c b/block/vvfat.c index 7b1dcee..22b586a 100644 --- a/block/vvfat.c +++ b/block/vvfat.c @@ -1162,7 +1162,7 @@ static int open_file(BDRVVVFATState* s,mapping_t* mapping) if(!s->current_mapping || strcmp(s->current_mapping->path,mapping->path)) { /* open file */ - int fd = open(mapping->path, O_RDONLY | O_BINARY | O_LARGEFILE); + int fd = qemu_open(mapping->path, O_RDONLY | O_BINARY | O_LARGEFILE); if(fd<0) return -1; vvfat_close_current_file(s); @@ -2221,7 +2221,7 @@ static int commit_one_file(BDRVVVFATState* s, for (i = s->cluster_size; i < offset; i += s->cluster_size) c = modified_fat_get(s, c); - fd = open(mapping->path, O_RDWR | O_CREAT | O_BINARY, 0666); + fd = qemu_open(mapping->path, O_RDWR | O_CREAT | O_BINARY, 0666); if (fd < 0) { fprintf(stderr, "Could not open %s... (%s, %d)\n", mapping->path, strerror(errno), errno); -- 1.7.10.4

On 07/25/2012 03:22 PM, Eric Blake wrote: It is adding O_CLOEXEC in qemu_open() on the open() call (as long as it is defined). The statement in the commit message isn't referring to anything dealing with qemu_dup(). The statement is specifically talking about the open() call in qemu_open(), and the point is to alert folks that old open() calls are now being routed through qemu_open() and likely are using the O_CLOEXEC flag, which is new behavior. That's right. qemu_open() adds O_CLOEXEC on the open() call. I have a note to get rid of O_LARGEFILE as a separate follow-on patch. Yep. -- Regards, Corey

When qemu_open is passed a filename of the "/dev/fdset/nnn" format (where nnn is the fdset ID), an fd with matching access mode flags will be searched for within the specified monitor fd set. If the fd is found, a dup of the fd will be returned from qemu_open. Each fd set has a reference count. The purpose of the reference count is to determine if an fd set contains file descriptors that have open dup() references that have not yet been closed. It is incremented on qemu_open and decremented on qemu_close. It is not until the refcount is zero that file desriptors in an fd set can be closed. If an fd set has dup() references open, then we must keep the other fds in the fd set open in case a reopen of the file occurs that requires an fd with a different access mode. Signed-off-by: Corey Bryant <coreyb(a)linux.vnet.ibm.com&gt; v2: -Get rid of file_open and move dup code to qemu_open (kwolf(a)redhat.com) -Use strtol wrapper instead of atoi (kwolf(a)redhat.com) v3: -Add note about fd leakage (eblake(a)redhat.com) v4 -Moved patch to be later in series (lcapitulino(a)redhat.com) -Update qemu_open to check access mode flags and set flags that can be set (eblake(a)redhat.com, kwolf(a)redhat.com) v5: -This patch was overhauled quite a bit in this version, with the addition of fd set and refcount support. -Use qemu_set_cloexec() on dup'd fd (eblake(a)redhat.com) -Modify flags set by fcntl on dup'd fd (eblake(a)redhat.com) -Reduce syscalls when setting flags for dup'd fd (eblake(a)redhat.com) -Fix O_RDWR, O_RDONLY, O_WRONLY checks (eblake(a)redhat.com) --- block/raw-posix.c | 24 +++++----- block/raw-win32.c | 2 +- block/vmdk.c | 4 +- block/vpc.c | 2 +- block/vvfat.c | 12 ++--- cutils.c | 5 ++ monitor.c | 85 +++++++++++++++++++++++++++++++++ monitor.h | 4 ++ osdep.c | 138 ++++++++++++++++++++++++++++++++++++++++++++++++++++- qemu-common.h | 3 +- qemu-tool.c | 12 +++++ 11 files changed, 267 insertions(+), 24 deletions(-) diff --git a/block/raw-posix.c b/block/raw-posix.c index a172de3..5d0a801 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: - qemu_close(fd); + qemu_close(fd, filename); return -errno; } @@ -376,7 +376,7 @@ static void raw_close(BlockDriverState *bs) { BDRVRawState *s = bs->opaque; if (s->fd >= 0) { - qemu_close(s->fd); + qemu_close(s->fd, bs->filename); 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 (qemu_close(fd) != 0) { + if (qemu_close(fd, filename) != 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 { - qemu_close(fd); + qemu_close(fd, bsdPath); } 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) { - qemu_close(s->fd); + qemu_close(s->fd, bs->filename); 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; - qemu_close(fd); + qemu_close(fd, filename); 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 */ - qemu_close(s->fd); + qemu_close(s->fd, filename); s->fd = -1; s->fd_media_changed = 1; @@ -1070,7 +1070,7 @@ static int floppy_probe_device(const char *filename) prio = 100; outc: - qemu_close(fd); + qemu_close(fd, filename); out: return prio; } @@ -1105,14 +1105,14 @@ static void floppy_eject(BlockDriverState *bs, bool eject_flag) int fd; if (s->fd >= 0) { - qemu_close(s->fd); + qemu_close(s->fd, bs->filename); s->fd = -1; } fd = qemu_open(bs->filename, s->open_flags | O_NONBLOCK); if (fd >= 0) { if (ioctl(fd, FDEJECT, 0) < 0) perror("FDEJECT"); - qemu_close(fd); + qemu_close(fd, bs->filename); } } @@ -1173,7 +1173,7 @@ static int cdrom_probe_device(const char *filename) prio = 100; outc: - qemu_close(fd); + qemu_close(fd, filename); out: return prio; } @@ -1281,7 +1281,7 @@ static int cdrom_reopen(BlockDriverState *bs) * FreeBSD seems to not notice sometimes... */ if (s->fd >= 0) - qemu_close(s->fd); + qemu_close(s->fd, bs->filename); 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 c56bf83..b795871 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); - qemu_close(fd); + qemu_close(fd, filename); return 0; } diff --git a/block/vmdk.c b/block/vmdk.c index daee426..7f1206b 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: - qemu_close(fd); + qemu_close(fd, filename); return ret; } @@ -1506,7 +1506,7 @@ static int vmdk_create(const char *filename, QEMUOptionParameter *options) } ret = 0; exit: - qemu_close(fd); + qemu_close(fd, filename); return ret; } diff --git a/block/vpc.c b/block/vpc.c index c0b82c4..20f648a 100644 --- a/block/vpc.c +++ b/block/vpc.c @@ -744,7 +744,7 @@ static int vpc_create(const char *filename, QEMUOptionParameter *options) } fail: - qemu_close(fd); + qemu_close(fd, filename); return ret; } diff --git a/block/vvfat.c b/block/vvfat.c index 59d3c5b..cbc7543 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) { - qemu_close(s->current_fd); + qemu_close(s->current_fd, s->current_mapping->path); 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) { - qemu_close(fd); + qemu_close(fd, mapping->path); 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) { - qemu_close(fd); + qemu_close(fd, mapping->path); g_free(cluster); return ret; } if (write(fd, cluster, rest_size) < 0) { - qemu_close(fd); + qemu_close(fd, mapping->path); g_free(cluster); return -2; } @@ -2268,11 +2268,11 @@ static int commit_one_file(BDRVVVFATState* s, if (ftruncate(fd, size)) { perror("ftruncate()"); - qemu_close(fd); + qemu_close(fd, mapping->path); g_free(cluster); return -4; } - qemu_close(fd); + qemu_close(fd, mapping->path); g_free(cluster); return commit_mappings(s, first_cluster, dir_index); diff --git a/cutils.c b/cutils.c index e2bc1b8..4e6bfdc 100644 --- a/cutils.c +++ b/cutils.c @@ -375,3 +375,8 @@ int qemu_parse_fd(const char *param) } return fd; } + +int qemu_parse_fdset(const char *param) +{ + return qemu_parse_fd(param); +} diff --git a/monitor.c b/monitor.c index 30b085f..fddd2b5 100644 --- a/monitor.c +++ b/monitor.c @@ -2551,6 +2551,91 @@ static void monitor_fdsets_set_in_use(Monitor *mon, bool in_use) } } +void monitor_fdset_increment_refcount(Monitor *mon, int64_t fdset_id) +{ + mon_fdset_t *mon_fdset; + + if (!mon) { + return; + } + + QLIST_FOREACH(mon_fdset, &mon->fdsets, next) { + if (mon_fdset->id == fdset_id) { + mon_fdset->refcount++; + break; + } + } +} + +void monitor_fdset_decrement_refcount(Monitor *mon, int64_t fdset_id) +{ + mon_fdset_t *mon_fdset; + + if (!mon) { + return; + } + + QLIST_FOREACH(mon_fdset, &mon->fdsets, next) { + if (mon_fdset->id == fdset_id) { + mon_fdset->refcount--; + if (mon_fdset->refcount == 0) { + monitor_fdset_cleanup(mon_fdset); + } + break; + } + } +} + +int monitor_fdset_get_fd(Monitor *mon, int64_t fdset_id, int flags) +{ + mon_fdset_t *mon_fdset; + mon_fdset_fd_t *mon_fdset_fd; + int mon_fd_flags; + + if (!mon) { + errno = ENOENT; + return -1; + } + + QLIST_FOREACH(mon_fdset, &mon->fdsets, next) { + if (mon_fdset->id != fdset_id) { + continue; + } + QLIST_FOREACH(mon_fdset_fd, &mon_fdset->fds, next) { + if (mon_fdset_fd->removed) { + continue; + } + + mon_fd_flags = fcntl(mon_fdset_fd->fd, F_GETFL); + if (mon_fd_flags == -1) { + return -1; + } + + switch (flags & O_ACCMODE) { + case O_RDWR: + if ((mon_fd_flags & O_ACCMODE) == O_RDWR) { + return mon_fdset_fd->fd; + } + break; + case O_RDONLY: + if ((mon_fd_flags & O_ACCMODE) == O_RDONLY) { + return mon_fdset_fd->fd; + } + break; + case O_WRONLY: + if ((mon_fd_flags & O_ACCMODE) == O_WRONLY) { + return mon_fdset_fd->fd; + } + break; + } + } + errno = EACCES; + return -1; + } + errno = ENOENT; + return -1; +} + /* mon_cmds and info_cmds would be sorted at runtime */ static mon_cmd_t mon_cmds[] = { #include "hmp-commands.h" diff --git a/monitor.h b/monitor.h index 5f4de1b..1efed38 100644 --- a/monitor.h +++ b/monitor.h @@ -86,4 +86,8 @@ int qmp_qom_set(Monitor *mon, const QDict *qdict, QObject **ret); int qmp_qom_get(Monitor *mon, const QDict *qdict, QObject **ret); +int monitor_fdset_get_fd(Monitor *mon, int64_t fdset_id, int flags); +void monitor_fdset_increment_refcount(Monitor *mon, int64_t fdset_id); +void monitor_fdset_decrement_refcount(Monitor *mon, int64_t fdset_id); + #endif /* !MONITOR_H */ diff --git a/osdep.c b/osdep.c index 7b09dff..0303cdd 100644 --- a/osdep.c +++ b/osdep.c @@ -47,6 +47,7 @@ extern int madvise(caddr_t, size_t, int); #include "qemu-common.h" #include "trace.h" #include "qemu_socket.h" +#include "monitor.h" static const char *qemu_version = QEMU_VERSION; @@ -75,6 +76,79 @@ int qemu_madvise(void *addr, size_t len, int advice) #endif } +/* + * Dups an fd and sets the flags + */ +static int qemu_dup(int fd, int flags) +{ + int i; + int ret; + int serrno; + int dup_flags; + int setfl_flags[] = { O_APPEND, O_ASYNC, O_DIRECT, O_NOATIME, + O_NONBLOCK, 0 }; + + if (flags & O_CLOEXEC) { + ret = fcntl(fd, F_DUPFD_CLOEXEC, 0); + if (ret == -1 && errno == EINVAL) { + ret = dup(fd); + if (ret != -1 && fcntl_setfl(ret, O_CLOEXEC) == -1) { + goto fail; + } + } + } else { + ret = dup(fd); + } + + if (ret == -1) { + goto fail; + } + + dup_flags = fcntl(ret, F_GETFL); + if (dup_flags == -1) { + goto fail; + } + + if ((flags & O_SYNC) != (dup_flags & O_SYNC)) { + errno = EINVAL; + goto fail; + } + + /* Set/unset flags that we can with fcntl */ + i = 0; + while (setfl_flags[i] != 0) { + if (flags & setfl_flags[i]) { + dup_flags = (dup_flags | setfl_flags[i]); + } else { + dup_flags = (dup_flags & ~setfl_flags[i]); + } + i++; + } + + if (fcntl(ret, F_SETFL, dup_flags) == -1) { + goto fail; + } + + /* Truncate the file in the cases that open() would truncate it */ + if (flags & O_TRUNC || + ((flags & (O_CREAT | O_EXCL)) == (O_CREAT | O_EXCL))) { + if (ftruncate(ret, 0) == -1) { + goto fail; + } + } + + qemu_set_cloexec(ret); + + return ret; + +fail: + serrno = errno; + if (ret != -1) { + close(ret); + } + errno = serrno; + return -1; +} /* * Opens a file with FD_CLOEXEC set @@ -84,6 +158,36 @@ int qemu_open(const char *name, int flags, ...) int ret; int mode = 0; +#ifndef _WIN32 + const char *fdset_id_str; + + /* Attempt dup of fd from fd set */ + if (strstart(name, "/dev/fdset/", &fdset_id_str)) { + int64_t fdset_id; + int fd, dupfd; + + fdset_id = qemu_parse_fdset(fdset_id_str); + if (fdset_id == -1) { + errno = EINVAL; + return -1; + } + + fd = monitor_fdset_get_fd(default_mon, fdset_id, flags); + if (fd == -1) { + return -1; + } + + dupfd = qemu_dup(fd, flags); + if (fd == -1) { + return -1; + } + + monitor_fdset_increment_refcount(default_mon, fdset_id); + + return dupfd; + } +#endif + if (flags & O_CREAT) { va_list ap; @@ -104,8 +208,40 @@ int qemu_open(const char *name, int flags, ...) return ret; } -int qemu_close(int fd) +int qemu_close(int fd, const char *name) { + const char *fdset_id_str; + + /* Close fd that was dup'd from an fdset */ + if (strstart(name, "/dev/fdset/", &fdset_id_str)) { + int ret; + int64_t fdset_id; + int fdset_fd; + int flags; + + fdset_id = qemu_parse_fdset(fdset_id_str); + if (fdset_id == -1) { + return -1; + } + + flags = fcntl(fd, F_GETFL); + if (flags == -1) { + return -1; + } + + fdset_fd = monitor_fdset_get_fd(default_mon, fdset_id, flags); + if (fdset_fd == -1) { + return -1; + } + + ret = close(fd); + if (ret == 0) { + monitor_fdset_decrement_refcount(default_mon, fdset_id); + } + + return ret; + } + return close(fd); } diff --git a/qemu-common.h b/qemu-common.h index c8ddf2f..6b4123c 100644 --- a/qemu-common.h +++ b/qemu-common.h @@ -147,6 +147,7 @@ int qemu_fls(int i); int qemu_fdatasync(int fd); int fcntl_setfl(int fd, int flag); int qemu_parse_fd(const char *param); +int qemu_parse_fdset(const char *param); /* * strtosz() suffixes used to specify the default treatment of an @@ -188,7 +189,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); +int qemu_close(int fd, const char *name); 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/qemu-tool.c b/qemu-tool.c index 318c5fc..f4db9db 100644 --- a/qemu-tool.c +++ b/qemu-tool.c @@ -31,6 +31,7 @@ struct QEMUBH }; Monitor *cur_mon; +Monitor *default_mon; int monitor_cur_is_qmp(void) { @@ -57,6 +58,17 @@ void monitor_protocol_event(MonitorEvent event, QObject *data) { } +int monitor_fdset_get_fd(Monitor *mon, int64_t fdset_id, int flags) +{ + return -1; +} +void monitor_fdset_increment_refcount(Monitor *mon, int64_t fdset_id) +{ +} +void monitor_fdset_decrement_refcount(Monitor *mon, int64_t fdset_id) +{ +} + int64_t cpu_get_clock(void) { return qemu_get_clock_ns(rt_clock); -- 1.7.10.4

On 07/23/2012 09:14 AM, Corey Bryant wrote: Does it make sense to use default_mon here? After digging into this some more, I'm thinking it makes sense, and I'll explain why. It looks like cur_mon can't be used. cur_mon will point to the monitor object for the duration of a command, and be reset to old_mon (NULL in my case) after the command completes. qemu_open() and qemu_close() are frequently called long after a monitor command has come and gone, so cur_mon won't work. For example, drive_add will cause qemu_open() to be called, but after the command has completed, the file will keep getting opened/closed during normal QEMU operation. I'm not sure why, I've just noticed this behavior. Does anyone have any thoughts on this? It would require fd sets to be added to the default monitor only. -- Regards, Corey

On 08/06/2012 05:15 AM, Kevin Wolf wrote: I'd prefer to have them associated with a monitor object so that we can more easily keep track of whether or not they're in use by a monitor connection. Are you saying that each monitor would have the same copy of fdset information? What if we also allow them to be added only to the default monitor? -- Regards, Corey

On 08/06/2012 09:51 AM, Kevin Wolf wrote: I think global fdsets might work (sorry I didn't think it through enough on my first reply). I think I'll need to drop the "in_use" flag and tie monitor references into the refcount. (I know I know, you suggested that a while back.. :). I'll give it a shot and see how it goes. -- Regards, Corey

Am 23.07.2012 15:08, schrieb Corey Bryant: Hm, not a nice interface where qemu_close() needs the filename and (worse) could be given a wrong filename. Maybe it would be better to maintain a list of fd -> fdset mappings in qemu_open/close? But if we decided to keep it like this, please use the right interface from the beginning in patch 5 instead of updating it here. These two functions are almost the same. Would a monitor_fdset_update_refcount(mon, fdset_id, value) make sense? These functions could then be kept as thin wrappers around it, or they could even be dropped completely. I think you mean: if ((flags & O_ACCMODE) == (mon_fd_flags & O_ACCMODE)) { return mon_fdset_fd->fd; } What about other flags that cannot be set with fcntl(), like O_SYNC on older kernels or possibly non-Linux? (The block layer doesn't use it any more, but I think we want to keep the function generally useful) It's worth trying to set it before failing, newer kernels can do it. But as I said above, if you can fail here, it makes sense to consider O_SYNC when selecting the right file descriptor from the fdset. What about this instead of the loop: int setfl_flags = O_APPEND | O_ASYNC | ... ; dup_flags &= ~setfl_flags; dup_flags |= (flags & setfl_flags); O_CREAT | O_EXCL kind of loses its meaning here, but okay, it's hard to do better with file descriptors. Wait... If O_CLOEXEC is set, you set the flag immediately and if it isn't you set it at the end of the function? What's the intended use case for not using O_CLOEXEC then? Kevin

Am 25.07.2012 05:41, schrieb Corey Bryant: I don't think so. qemu_close() is not a fast path and happens almost never, and the list is short enough that searching it isn't a problem anyway. Now that the block layer doesn't use O_SYNC any more, it's more of a theoretical point. I don't think there's any other place, where we'd need to switch O_SYNC during runtime. Taking it into consideration is complicated by the fact that some kernels allow to fcntl() O_SYNC and others don't, so enforcing a match here wouldn't feel completely right either. Maybe just leave it as it is. :-) Hm, true. So it seems that patch has never made it into the kernel, in fact... Yes, probably. And in fact, I think this shouldn't even be conditional on flags & O_CLOEXEC. The whole reason qemu_open() was introduced originally was to always set O_CLOEXEC. Kevin

On 07/23/2012 07:08 AM, Corey Bryant wrote: Am I reading this code right by stating that 'if there is no monitor, we don't increment the refcount'? How does a monitor reattach affect things? Or am I missing something fundamental about the cases when 'mon==NULL' will exist? This says we fail on the first fcntl() failure, instead of trying other fds in the set. Granted, an fcntl() failure is probably the sign of a bigger bug (such as closing an fd at the wrong point in time), so I guess trying to go on doesn't make much sense once we already know we are hosed. Do we want to allow the case where the caller asked for O_RDONLY, but the set only has O_RDWR? After all, the caller is getting a compatible subset of what the set offers. Likewise, should we allow a caller asking for O_WRONLY when the set provides only O_RDWR? F_DUPFD_CLOEXEC is required by POSIX but not implemented on all modern OS yet; you probably need some #ifdef and/or configure guards. You _can't_ call F_SETFL with O_CLOEXEC. O_CLOEXEC only causes open() to set FD_CLOEXEC; thereafter, including in the case of this dup, what you want to do is instead set FD_CLOEXEC via F_SETFD (aka call qemu_set_cloexec, not fcntl_setfl). Rather than looping one bit at a time, you should do this as a mask operation. If we're going to blindly set FD_CLOEXEC at the end of the day, rather than try to honor O_CLOEXEC, then why not simplify the beginning of this function: ret = fcntl(fd, F_DUPFD_CLOEXEC, 0); if (ret == -1 && errno == EINVAL) { ret = dup(fd); if (ret != -1) { qemu_set_cloexec(ret); } } if (ret == -1) { goto fail; } -- Eric Blake eblake(a)redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org