This sets the close-on-exec flag for the file descriptor received via SCM_RIGHTS. Signed-off-by: Corey Bryant <coreyb(a)linux.vnet.ibm.com&gt; --- v4 -This patch is new in v4 (eblake(a)redhat.com) qemu-char.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/qemu-char.c b/qemu-char.c index c2aaaee..f890113 100644 --- a/qemu-char.c +++ b/qemu-char.c @@ -2263,7 +2263,7 @@ static ssize_t tcp_chr_recv(CharDriverState *chr, char *buf, size_t len) msg.msg_control = &msg_control; msg.msg_controllen = sizeof(msg_control); - ret = recvmsg(s->fd, &msg, 0); + ret = recvmsg(s->fd, &msg, MSG_CMSG_CLOEXEC); if (ret > 0 && s->is_unix) unix_process_msgfd(chr, &msg); -- 1.7.10.2

On Fri, Jun 22, 2012 at 01:31:17PM -0600, Eric Blake wrote: Good point. I think the answer is yes. Just like qemu_open() we can wrap "recvmsg(2) with fd" so that platforms with MSG_CMSG_CLOEXEC use that flag and other platforms use qemu_set_cloexec(). Stefan

Signed-off-by: Corey Bryant <coreyb(a)linux.vnet.ibm.com&gt; --- v2: -Convert getfd and closefd to QAPI (lcapitulino(a)redhat.com) -Remove changes that returned fd from getfd (lcapitulino(a)redhat.com) -Wrap hmp_* functions around qmp_* functions (kwolf(a)redhat.com) -Move hmp_* functions to hmp.c (lcapitulino(a)redhat.com) -Drop .user_print lines (lcapitulino(a)redhat.com) -Use 'cmd' instead of 'cmd_new' for HMP (lcapitulino(a)redhat.com) -Change QMP command existance back to 0.14 (lcapitulino(a)redhat.com) v3: -Remove unnecessary return statements from qmp_* functions (lcapitulino(a)redhat.com) -Add notes to QMP command describing behavior in more detail (lcapitulino(a)redhat.com, eblake(a)redhat.com) v4: -No changes hmp-commands.hx | 6 ++---- hmp.c | 18 ++++++++++++++++++ hmp.h | 2 ++ monitor.c | 32 ++++++++++++++------------------ qapi-schema.json | 35 +++++++++++++++++++++++++++++++++++ qmp-commands.hx | 14 ++++++++++---- 6 files changed, 81 insertions(+), 26 deletions(-) diff --git a/hmp-commands.hx b/hmp-commands.hx index f5d9d91..eea8b32 100644 --- a/hmp-commands.hx +++ b/hmp-commands.hx @@ -1236,8 +1236,7 @@ ETEXI .args_type = "fdname:s", .params = "getfd name", .help = "receive a file descriptor via SCM rights and assign it a name", - .user_print = monitor_user_noop, - .mhandler.cmd_new = do_getfd, + .mhandler.cmd = hmp_getfd, }, STEXI @@ -1253,8 +1252,7 @@ ETEXI .args_type = "fdname:s", .params = "closefd name", .help = "close a file descriptor previously passed via SCM rights", - .user_print = monitor_user_noop, - .mhandler.cmd_new = do_closefd, + .mhandler.cmd = hmp_closefd, }, STEXI diff --git a/hmp.c b/hmp.c index 2ce8cb9..6241856 100644 --- a/hmp.c +++ b/hmp.c @@ -999,3 +999,21 @@ void hmp_netdev_del(Monitor *mon, const QDict *qdict) qmp_netdev_del(id, &err); hmp_handle_error(mon, &err); } + +void hmp_getfd(Monitor *mon, const QDict *qdict) +{ + const char *fdname = qdict_get_str(qdict, "fdname"); + Error *errp = NULL; + + qmp_getfd(fdname, &errp); + hmp_handle_error(mon, &errp); +} + +void hmp_closefd(Monitor *mon, const QDict *qdict) +{ + const char *fdname = qdict_get_str(qdict, "fdname"); + Error *errp = NULL; + + qmp_closefd(fdname, &errp); + hmp_handle_error(mon, &errp); +} diff --git a/hmp.h b/hmp.h index 79d138d..8d2b0d7 100644 --- a/hmp.h +++ b/hmp.h @@ -64,5 +64,7 @@ void hmp_device_del(Monitor *mon, const QDict *qdict); void hmp_dump_guest_memory(Monitor *mon, const QDict *qdict); void hmp_netdev_add(Monitor *mon, const QDict *qdict); void hmp_netdev_del(Monitor *mon, const QDict *qdict); +void hmp_getfd(Monitor *mon, const QDict *qdict); +void hmp_closefd(Monitor *mon, const QDict *qdict); #endif diff --git a/monitor.c b/monitor.c index a3bc2c7..1a7f7e7 100644 --- a/monitor.c +++ b/monitor.c @@ -2182,48 +2182,45 @@ static void do_inject_mce(Monitor *mon, const QDict *qdict) } #endif -static int do_getfd(Monitor *mon, const QDict *qdict, QObject **ret_data) +void qmp_getfd(const char *fdname, Error **errp) { - const char *fdname = qdict_get_str(qdict, "fdname"); mon_fd_t *monfd; int fd; - fd = qemu_chr_fe_get_msgfd(mon->chr); + fd = qemu_chr_fe_get_msgfd(cur_mon->chr); if (fd == -1) { - qerror_report(QERR_FD_NOT_SUPPLIED); - return -1; + error_set(errp, QERR_FD_NOT_SUPPLIED); + return; } if (qemu_isdigit(fdname[0])) { - qerror_report(QERR_INVALID_PARAMETER_VALUE, "fdname", - "a name not starting with a digit"); - return -1; + error_set(errp, QERR_INVALID_PARAMETER_VALUE, "fdname", + "a name not starting with a digit"); + return; } - QLIST_FOREACH(monfd, &mon->fds, next) { + QLIST_FOREACH(monfd, &cur_mon->fds, next) { if (strcmp(monfd->name, fdname) != 0) { continue; } close(monfd->fd); monfd->fd = fd; - return 0; + return; } monfd = g_malloc0(sizeof(mon_fd_t)); monfd->name = g_strdup(fdname); monfd->fd = fd; - QLIST_INSERT_HEAD(&mon->fds, monfd, next); - return 0; + QLIST_INSERT_HEAD(&cur_mon->fds, monfd, next); } -static int do_closefd(Monitor *mon, const QDict *qdict, QObject **ret_data) +void qmp_closefd(const char *fdname, Error **errp) { - const char *fdname = qdict_get_str(qdict, "fdname"); mon_fd_t *monfd; - QLIST_FOREACH(monfd, &mon->fds, next) { + QLIST_FOREACH(monfd, &cur_mon->fds, next) { if (strcmp(monfd->name, fdname) != 0) { continue; } @@ -2232,11 +2229,10 @@ static int do_closefd(Monitor *mon, const QDict *qdict, QObject **ret_data) close(monfd->fd); g_free(monfd->name); g_free(monfd); - return 0; + return; } - qerror_report(QERR_FD_NOT_FOUND, fdname); - return -1; + error_set(errp, QERR_FD_NOT_FOUND, fdname); } static void do_loadvm(Monitor *mon, const QDict *qdict) diff --git a/qapi-schema.json b/qapi-schema.json index 3b6e346..26a6b84 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -1862,3 +1862,38 @@ # Since: 0.14.0 ## { 'command': 'netdev_del', 'data': {'id': 'str'} } + +## +# @getfd: +# +# Receive a file descriptor via SCM rights and assign it a name +# +# @fdname: file descriptor name +# +# Returns: Nothing on success +# If file descriptor was not received, FdNotSupplied +# If @fdname is not valid, InvalidParameterType +# +# Since: 0.14.0 +# +# Notes: If @fdname already exists, the file descriptor assigned to +# it will be closed and replaced by the received file +# descriptor. +# The 'closefd' command can be used to explicitly close the +# file descriptor when it is no longer needed. +## +{ 'command': 'getfd', 'data': {'fdname': 'str'} } + +## +# @closefd: +# +# Close a file descriptor previously passed via SCM rights +# +# @fdname: file descriptor name +# +# Returns: Nothing on success +# If @fdname is not found, FdNotFound +# +# Since: 0.14.0 +## +{ 'command': 'closefd', 'data': {'fdname': 'str'} } diff --git a/qmp-commands.hx b/qmp-commands.hx index 2e1a38e..e3cf3c5 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -873,8 +873,7 @@ EQMP .args_type = "fdname:s", .params = "getfd name", .help = "receive a file descriptor via SCM rights and assign it a name", - .user_print = monitor_user_noop, - .mhandler.cmd_new = do_getfd, + .mhandler.cmd_new = qmp_marshal_input_getfd, }, SQMP @@ -892,6 +891,14 @@ Example: -> { "execute": "getfd", "arguments": { "fdname": "fd1" } } <- { "return": {} } +Notes: + +(1) If the name specified by the "fdname" argument already exists, + the file descriptor assigned to it will be closed and replaced + by the received file descriptor. +(2) The 'closefd' command can be used to explicitly close the file + descriptor when it is no longer needed. + EQMP { @@ -899,8 +906,7 @@ EQMP .args_type = "fdname:s", .params = "closefd name", .help = "close a file descriptor previously passed via SCM rights", - .user_print = monitor_user_noop, - .mhandler.cmd_new = do_closefd, + .mhandler.cmd_new = qmp_marshal_input_closefd, }, SQMP -- 1.7.10.2

On 06/26/2012 04:44 PM, Luiz Capitulino wrote: Thanks! I'll do that. I'll assume we still want this patch even if pass-fd support is dropped. -- Regards, Corey

On 06/26/2012 05:28 PM, Luiz Capitulino wrote: It might just be easier if I keep it in my series for now, at least until we figure out what direction we're going to take regarding pass-fd vs passing fd's via SCM_RIGHTS with commands like drive_add, etc. -- Regards, Corey

This patch adds the pass-fd QMP command using the QAPI framework. Like the getfd command, it is used to pass a file descriptor via SCM_RIGHTS and associate it with a name. However, the pass-fd command also returns the received file descriptor, which is a difference in behavior from the getfd command, which returns nothing. pass-fd also supports a boolean "force" argument that can be used to specify that an existing file descriptor is associated with the specified name, and that it should become a copy of the newly passed file descriptor. The closefd command can be used to close a file descriptor that was passed with the pass-fd command. Note that when using getfd or pass-fd, there are some commands (e.g. migrate with fd:name) that implicitly close the named fd. When this is not the case, closefd must be used to avoid fd leaks. Signed-off-by: Corey Bryant <coreyb(a)linux.vnet.ibm.com&gt; --- v2: -Introduce new QMP command to pass/return fd (lcapitulino(a)redhat.com) -Use passfd as command name (berrange(a)redhat.com) v3: -Use pass-fd as command name (lcapitulino(a)redhat.com) -Fail pass-fd if fdname already exists (lcapitulino(a)redhat.com) -Add notes to QMP command describing behavior in more detail (lcapitulino(a)redhat.com, eblake(a)redhat.com) -Add note about fd leakage (eblake(a)redhat.com) v4: -Don't return same error class twice (lcapitulino(a)redhat.com) -Share code for qmp_gefd and qmp_pass_fd (lcapitulino(a)redhat.com) -Update qmp_closefd to call monitor_get_fd -Add support for "force" argument to pass-fd (eblake(a)redhat.com) dump.c | 3 +- migration-fd.c | 2 +- monitor.c | 96 +++++++++++++++++++++++++++++++++++------------------- monitor.h | 2 +- net.c | 6 ++-- qapi-schema.json | 36 ++++++++++++++++++++ qmp-commands.hx | 42 +++++++++++++++++++++++- 7 files changed, 146 insertions(+), 41 deletions(-) diff --git a/dump.c b/dump.c index 4412d7a..2fd8ced 100644 --- a/dump.c +++ b/dump.c @@ -836,9 +836,8 @@ void qmp_dump_guest_memory(bool paging, const char *file, bool has_begin, #if !defined(WIN32) if (strstart(file, "fd:", &p)) { - fd = monitor_get_fd(cur_mon, p); + fd = monitor_get_fd(cur_mon, p, errp); if (fd == -1) { - error_set(errp, QERR_FD_NOT_FOUND, p); return; } } diff --git a/migration-fd.c b/migration-fd.c index 50138ed..7335167 100644 --- a/migration-fd.c +++ b/migration-fd.c @@ -75,7 +75,7 @@ static int fd_close(MigrationState *s) int fd_start_outgoing_migration(MigrationState *s, const char *fdname) { - s->fd = monitor_get_fd(cur_mon, fdname); + s->fd = monitor_get_fd(cur_mon, fdname, NULL); if (s->fd == -1) { DPRINTF("fd_migration: invalid file descriptor identifier\n"); goto err_after_get_fd; diff --git a/monitor.c b/monitor.c index 1a7f7e7..3433c06 100644 --- a/monitor.c +++ b/monitor.c @@ -814,7 +814,7 @@ static int add_graphics_client(Monitor *mon, const QDict *qdict, QObject **ret_d CharDriverState *s; if (strcmp(protocol, "spice") == 0) { - int fd = monitor_get_fd(mon, fdname); + int fd = monitor_get_fd(mon, fdname, NULL); int skipauth = qdict_get_try_bool(qdict, "skipauth", 0); int tls = qdict_get_try_bool(qdict, "tls", 0); if (!using_spice) { @@ -828,18 +828,18 @@ static int add_graphics_client(Monitor *mon, const QDict *qdict, QObject **ret_d return 0; #ifdef CONFIG_VNC } else if (strcmp(protocol, "vnc") == 0) { - int fd = monitor_get_fd(mon, fdname); + int fd = monitor_get_fd(mon, fdname, NULL); int skipauth = qdict_get_try_bool(qdict, "skipauth", 0); - vnc_display_add_client(NULL, fd, skipauth); - return 0; + vnc_display_add_client(NULL, fd, skipauth); + return 0; #endif } else if ((s = qemu_chr_find(protocol)) != NULL) { - int fd = monitor_get_fd(mon, fdname); - if (qemu_chr_add_client(s, fd) < 0) { - qerror_report(QERR_ADD_CLIENT_FAILED); - return -1; - } - return 0; + int fd = monitor_get_fd(mon, fdname, NULL); + if (qemu_chr_add_client(s, fd) < 0) { + qerror_report(QERR_ADD_CLIENT_FAILED); + return -1; + } + return 0; } qerror_report(QERR_INVALID_PARAMETER, "protocol"); @@ -2182,57 +2182,69 @@ static void do_inject_mce(Monitor *mon, const QDict *qdict) } #endif -void qmp_getfd(const char *fdname, Error **errp) +static int monitor_put_fd(Monitor *mon, const char *fdname, bool replace, + bool copy, Error **errp) { mon_fd_t *monfd; int fd; - fd = qemu_chr_fe_get_msgfd(cur_mon->chr); + fd = qemu_chr_fe_get_msgfd(mon->chr); if (fd == -1) { error_set(errp, QERR_FD_NOT_SUPPLIED); - return; + return -1; } if (qemu_isdigit(fdname[0])) { error_set(errp, QERR_INVALID_PARAMETER_VALUE, "fdname", "a name not starting with a digit"); - return; + return -1; } - QLIST_FOREACH(monfd, &cur_mon->fds, next) { + QLIST_FOREACH(monfd, &mon->fds, next) { if (strcmp(monfd->name, fdname) != 0) { continue; } - close(monfd->fd); - monfd->fd = fd; - return; + if (replace) { + /* the existing fd is replaced with the new fd */ + close(monfd->fd); + monfd->fd = fd; + return fd; + } + + if (copy) { + /* the existing fd becomes a copy of the new fd */ + if (dup2(fd, monfd->fd) == -1) { + error_set(errp, QERR_UNDEFINED_ERROR); + return -1; + } + close(fd); + return monfd->fd; + } + + error_set(errp, QERR_INVALID_PARAMETER, "fdname"); + return -1; + } + + if (copy) { + error_set(errp, QERR_INVALID_PARAMETER, "fdname"); + return -1; } monfd = g_malloc0(sizeof(mon_fd_t)); monfd->name = g_strdup(fdname); monfd->fd = fd; - QLIST_INSERT_HEAD(&cur_mon->fds, monfd, next); + QLIST_INSERT_HEAD(&mon->fds, monfd, next); + return fd; } void qmp_closefd(const char *fdname, Error **errp) { - mon_fd_t *monfd; - - QLIST_FOREACH(monfd, &cur_mon->fds, next) { - if (strcmp(monfd->name, fdname) != 0) { - continue; - } - - QLIST_REMOVE(monfd, next); - close(monfd->fd); - g_free(monfd->name); - g_free(monfd); - return; + int fd = monitor_get_fd(cur_mon, fdname, errp); + if (fd != -1) { + close(fd); } - - error_set(errp, QERR_FD_NOT_FOUND, fdname); } static void do_loadvm(Monitor *mon, const QDict *qdict) @@ -2247,7 +2259,7 @@ static void do_loadvm(Monitor *mon, const QDict *qdict) } } -int monitor_get_fd(Monitor *mon, const char *fdname) +int monitor_get_fd(Monitor *mon, const char *fdname, Error **errp) { mon_fd_t *monfd; @@ -2268,9 +2280,25 @@ int monitor_get_fd(Monitor *mon, const char *fdname) return fd; } + error_set(errp, QERR_FD_NOT_FOUND, fdname); return -1; } +int64_t qmp_pass_fd(const char *fdname, bool has_force, bool force, + Error **errp) +{ + bool replace = false; + bool copy = has_force ? force : false; + return monitor_put_fd(cur_mon, fdname, replace, copy, errp); +} + +void qmp_getfd(const char *fdname, Error **errp) +{ + bool replace = true; + bool copy = false; + monitor_put_fd(cur_mon, fdname, replace, copy, errp); +} + /* 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 cd1d878..8fa515d 100644 --- a/monitor.h +++ b/monitor.h @@ -63,7 +63,7 @@ int monitor_read_block_device_key(Monitor *mon, const char *device, BlockDriverCompletionFunc *completion_cb, void *opaque); -int monitor_get_fd(Monitor *mon, const char *fdname); +int monitor_get_fd(Monitor *mon, const char *fdname, Error **errp); void monitor_vprintf(Monitor *mon, const char *fmt, va_list ap) GCC_FMT_ATTR(2, 0); diff --git a/net.c b/net.c index 4aa416c..28860b8 100644 --- a/net.c +++ b/net.c @@ -730,12 +730,14 @@ int qemu_find_nic_model(NICInfo *nd, const char * const *models, int net_handle_fd_param(Monitor *mon, const char *param) { int fd; + Error *local_err = NULL; if (!qemu_isdigit(param[0]) && mon) { - fd = monitor_get_fd(mon, param); + fd = monitor_get_fd(mon, param, &local_err); if (fd == -1) { - error_report("No file descriptor named %s found", param); + qerror_report_err(local_err); + error_free(local_err); return -1; } } else { diff --git a/qapi-schema.json b/qapi-schema.json index 26a6b84..2ac1261 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -1864,6 +1864,41 @@ { 'command': 'netdev_del', 'data': {'id': 'str'} } ## +# @pass-fd: +# +# Pass a file descriptor via SCM rights and assign it a name +# +# @fdname: file descriptor name +# +# @force: #optional true specifies that an existing file descriptor is +# associated with @fdname, and that it should become a copy of the +# newly passed file descriptor. +# +# Returns: The QEMU file descriptor that is assigned to @fdname +# If file descriptor was not received, FdNotSupplied +# If @fdname is not valid, InvalidParameterValue +# If @fdname already exists, and @force is not true, InvalidParameter +# If @fdname does not already exist, and @force is true, +# InvalidParameter +# If @force fails to copy the passed file descriptor, +# UndefinedError +# +# Since: 1.2.0 +# +# Notes: If @fdname already exists, and @force is not true, the +# command will fail. +# +# If @fdname already exists, and @force is true, the value of +# the existing file descriptor is returned when the command is +# successful. +# +# The 'closefd' command can be used to explicitly close the +# file descriptor when it is no longer needed. +## +{ 'command': 'pass-fd', 'data': {'fdname': 'str', '*force': 'bool'}, + 'returns': 'int' } + +## # @getfd: # # Receive a file descriptor via SCM rights and assign it a name @@ -1879,6 +1914,7 @@ # Notes: If @fdname already exists, the file descriptor assigned to # it will be closed and replaced by the received file # descriptor. +# # The 'closefd' command can be used to explicitly close the # file descriptor when it is no longer needed. ## diff --git a/qmp-commands.hx b/qmp-commands.hx index e3cf3c5..7e3c07e 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -869,9 +869,49 @@ Example: EQMP { + .name = "pass-fd", + .args_type = "fdname:s,force:b?", + .params = "pass-fd fdname force", + .help = "pass a file descriptor via SCM rights and assign it a name", + .mhandler.cmd_new = qmp_marshal_input_pass_fd, + }, + +SQMP +pass-fd +------- + +Pass a file descriptor via SCM rights and assign it a name. + +Arguments: + +- "fdname": file descriptor name (json-string) +- "force": true specifies that an existing file descriptor is associated + with "fdname", and that it should become a copy of the newly + passed file descriptor. (json-bool, optional) + +Return a json-int with the QEMU file descriptor that is assigned to "fdname". + +Example: + +-> { "execute": "pass-fd", "arguments": { "fdname": "fd1" } } +<- { "return": 42 } + +Notes: + +(1) If the name specified by the "fdname" argument already exists, and + "force" is not true, the command will fail. +(2) If the name specified by the "fdname" argument already exists, and + "force" is true, the value of the existing file descriptor is + returned when the command is successful. +(3) The 'closefd' command can be used to explicitly close the file + descriptor when it is no longer needed. + +EQMP + + { .name = "getfd", .args_type = "fdname:s", - .params = "getfd name", + .params = "getfd fdname", .help = "receive a file descriptor via SCM rights and assign it a name", .mhandler.cmd_new = qmp_marshal_input_getfd, }, -- 1.7.10.2

Signed-off-by: Corey Bryant <coreyb(a)linux.vnet.ibm.com&gt; --- v4: -This patch is new in v4. monitor.c | 40 ++++++++++++++++++++-------------------- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/monitor.c b/monitor.c index 3433c06..153e949 100644 --- a/monitor.c +++ b/monitor.c @@ -2239,26 +2239,6 @@ static int monitor_put_fd(Monitor *mon, const char *fdname, bool replace, return fd; } -void qmp_closefd(const char *fdname, Error **errp) -{ - int fd = monitor_get_fd(cur_mon, fdname, errp); - if (fd != -1) { - close(fd); - } -} - -static void do_loadvm(Monitor *mon, const QDict *qdict) -{ - int saved_vm_running = runstate_is_running(); - const char *name = qdict_get_str(qdict, "name"); - - vm_stop(RUN_STATE_RESTORE_VM); - - if (load_vmstate(name) == 0 && saved_vm_running) { - vm_start(); - } -} - int monitor_get_fd(Monitor *mon, const char *fdname, Error **errp) { mon_fd_t *monfd; @@ -2299,6 +2279,26 @@ void qmp_getfd(const char *fdname, Error **errp) monitor_put_fd(cur_mon, fdname, replace, copy, errp); } +void qmp_closefd(const char *fdname, Error **errp) +{ + int fd = monitor_get_fd(cur_mon, fdname, errp); + if (fd != -1) { + close(fd); + } +} + +static void do_loadvm(Monitor *mon, const QDict *qdict) +{ + int saved_vm_running = runstate_is_running(); + const char *name = qdict_get_str(qdict, "name"); + + vm_stop(RUN_STATE_RESTORE_VM); + + if (load_vmstate(name) == 0 && saved_vm_running) { + vm_start(); + } +} + /* mon_cmds and info_cmds would be sorted at runtime */ static mon_cmd_t mon_cmds[] = { #include "hmp-commands.h" -- 1.7.10.2

On 06/26/2012 04:45 PM, Luiz Capitulino wrote: This just made patch 3/7 easier to review. -- Regards, Corey

On 06/26/2012 05:28 PM, Luiz Capitulino wrote: Sure, I can do that. -- Regards, Corey

This patch converts all block layer open calls to qemu_open. This enables all block layer open paths to dup(X) a pre-opened file descriptor if the filename is of the format /dev/fd/X. This is useful if QEMU is restricted from opening certain files. 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: -No changes v4: -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 | 21 +++++++++++---------- 6 files changed, 35 insertions(+), 36 deletions(-) diff --git a/block/raw-posix.c b/block/raw-posix.c index 0d9be0b..68886cd 100644 --- a/block/raw-posix.c +++ b/block/raw-posix.c @@ -568,8 +568,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 { @@ -741,7 +741,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 { @@ -798,7 +798,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; @@ -872,7 +872,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; @@ -952,7 +952,7 @@ static int floppy_probe_device(const char *filename) prio = 50; } - fd = open(filename, O_RDONLY | O_NONBLOCK); + fd = qemu_open(filename, O_RDONLY | O_NONBLOCK); if (fd < 0) { goto out; } @@ -1005,7 +1005,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"); @@ -1055,7 +1055,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; } @@ -1179,7 +1179,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 119d3c7..6183fdf 100644 --- a/block/vdi.c +++ b/block/vdi.c @@ -648,8 +648,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 0fd3367..81c3a19 100644 --- a/block/vvfat.c +++ b/block/vvfat.c @@ -1152,16 +1152,17 @@ static inline mapping_t* find_mapping_for_cluster(BDRVVVFATState* s,int cluster_ static int open_file(BDRVVVFATState* s,mapping_t* mapping) { if(!mapping) - return -1; + return -1; if(!s->current_mapping || - strcmp(s->current_mapping->path,mapping->path)) { - /* open file */ - int fd = open(mapping->path, O_RDONLY | O_BINARY | O_LARGEFILE); - if(fd<0) - return -1; - vvfat_close_current_file(s); - s->current_fd = fd; - s->current_mapping = mapping; + strcmp(s->current_mapping->path, mapping->path)) { + /* open file */ + int fd = qemu_open(mapping->path, O_RDONLY | O_BINARY | O_LARGEFILE); + if (fd < 0) { + return -1; + } + vvfat_close_current_file(s); + s->current_fd = fd; + s->current_mapping = mapping; } return 0; } @@ -2215,7 +2216,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.2

On Tue, Jun 26, 2012 at 03:11:40PM +0100, Daniel P. Berrange wrote: Here is a quick proof of concept (ie untested) patch to demonstrate what I mean. It relies on Cory's patch which converts everything to use qemu_open. It is also still valuable to make the change to qemu_open() to support "/dev/fd/N" for passing FDs during QEMU initial startup for CLI args. IMHO, what I propose here is preferrable for QMP clients that our current plan of requiring use of 3 monitor commands (passfd, XXXXX, closefd). From: "Daniel P. Berrange" <berrange(a)redhat.com&gt; Date: Tue, 26 Jun 2012 15:48:13 +0100 Subject: [PATCH] Support passing FDs with the monitor command that uses them Add support for a syntax "/dev/fdlist/N", where 'N' refers to the N'th FD that was passed along with the currently processing command in this thread. The QMP client sends { "execute": "drive_add", "arguments": { "filename": "/dev/fdlist/0", "backingfilename": "/dev/fdlist/1" }, "nfds": 2 } followed by "nfds" * SCM_RIGHTS packets The FDs received along with the "drive_add" command are placed in a thread-local list, then the QMP handler function is invoked. The qemu_open() function will resolve any path "/dev/fdlist/0" to use an FD stored in the thread-local list, and dup() it. One the QMP handler function is finished, all FDs in the thread local list are closed. THe reason for choice of "/dev/fdlist/N", is to allow the "/dev/fd/N" syntax to be used for FDs that are passed to QEMU at startup time, which will be in the global FD number namespace, not the thread-local index. The reason for using a thread local is to ensure the FDs are only made available to the currently executing monitor command handler in this thread, and not any other threads. The advantage over using a separate 'passfd' command, is that this guarentees all FDs are closed in QEMU once the QMP command handler is finished. There is no risk of leaks being triggered by the client either intentionally, or via it crashing. NB: this is a proof of concept demonstration of the overall architectural design. The patch would need more work before it was suitable for merging - Pick a better place/file for the fdlist APIs - Do proper error reporting in various places - Check for possibility that qemu_chr_fe_get_msgfd can block or return EAGAIN. diff --git a/monitor.c b/monitor.c index f6107ba..a3a4bd4 100644 --- a/monitor.c +++ b/monitor.c @@ -4490,6 +4490,8 @@ static void handle_qmp_command(JSONMessageParser *parser, QList *tokens) const mon_cmd_t *cmd; const char *cmd_name; Monitor *mon = cur_mon; + int nfds; + size_t i; args = input = NULL; @@ -4535,6 +4537,17 @@ static void handle_qmp_command(JSONMessageParser *parser, QList *tokens) goto err_out; } + nfds = qdict_get_int(input, "nfds"); + for (i = 0 ; i < nfds ; i++) { + int fd = qemu_chr_fe_get_msgfd(mon->chr); + if (fd == -1) { + qerror_report(QERR_FD_NOT_SUPPLIED); + goto err_out; + } + + qemu_fdlist_add(fd); + } + if (handler_is_async(cmd)) { err = qmp_async_cmd_handler(mon, cmd, args); if (err) { @@ -4552,6 +4565,7 @@ err_out: out: QDECREF(input); QDECREF(args); + qemu_fdlist_closeall(); } /** diff --git a/osdep.c b/osdep.c index 03817f0..0849cb6 100644 --- a/osdep.c +++ b/osdep.c @@ -75,6 +75,81 @@ int qemu_madvise(void *addr, size_t len, int advice) #endif } +typedef struct QEMUFDList { + int *fds; + size_t nfds; +} QEMUFDList; + +static pthread_key_t fdlist_key; + +static void qemu_fdlist_cleanup(void *data) +{ + size_t i; + QEMUFDList *fdlist = data; + + if (!fdlist) + return; + + for (i = 0 ; i < fdlist->nfds ; i++) { + close(fdlist->fds[i]); + } +} + +int qemu_fdlist_init(void) +{ + if (pthread_key_create(&fdlist_key, + qemu_fdlist_cleanup) < 0) + return -1; + + return 0; +} + +static QEMUFDList *qemu_fdlist_for_thread(void) +{ + QEMUFDList *fdlist = pthread_getspecific(fdlist_key); + + if (fdlist == NULL) { + fdlist = g_new0(QEMUFDList, 1); + pthread_setspecific(fdlist_key, fdlist); + } + + return fdlist; +} + +void qemu_fdlist_add(int fd) +{ + QEMUFDList *fdlist = qemu_fdlist_for_thread(); + + fdlist->fds = g_renew(int, fdlist->fds, fdlist->nfds + 1); + fdlist->fds[fdlist->nfds++] = fd; +} + +int qemu_fdlist_dup(int idx) +{ + QEMUFDList *fdlist = qemu_fdlist_for_thread(); + + if (idx >= fdlist->nfds) { + errno = EINVAL; + return -1; + } + + return dup(fdlist->fds[idx]); +} + +void qemu_fdlist_closeall(void) +{ + QEMUFDList *fdlist = qemu_fdlist_for_thread(); + size_t i; + + for (i = 0 ; i < fdlist->nfds ; i++) { + close(fdlist->fds[i]); + } + g_free(fdlist->fds); + fdlist->fds = NULL; + fdlist->nfds = 0; +} + + /* * Opens a file with FD_CLOEXEC set @@ -83,6 +158,26 @@ int qemu_open(const char *name, int flags, ...) { int ret; int mode = 0; + const char *p; + + /* Attempt dup of fd for pre-opened file */ + if (strstart(name, "/dev/fdlist/", &p)) { + int idx; + int fd; + + idx = qemu_parse_fd(p); + if (idx == -1) { + return -1; + } + + fd = qemu_fdlist_dup(idx); + if (fd == -1) { + return -1; + } + + /* XXX verify flags match */ + return fd; + } if (flags & O_CREAT) { va_list ap; diff --git a/qemu-common.h b/qemu-common.h index 8f87e41..b6b7203 100644 --- a/qemu-common.h +++ b/qemu-common.h @@ -194,6 +194,11 @@ ssize_t qemu_send_full(int fd, const void *buf, size_t count, int flags) ssize_t qemu_recv_full(int fd, void *buf, size_t count, int flags) QEMU_WARN_UNUSED_RESULT; +int qemu_fdlist_init(void); +void qemu_fdlist_add(int fd); +int qemu_fdlist_dup(int idx); +void qemu_fdlist_closeall(void); + #ifndef _WIN32 int qemu_eventfd(int pipefd[2]); int qemu_pipe(int pipefd[2]); diff --git a/vl.c b/vl.c index 1329c30..e515c84 100644 --- a/vl.c +++ b/vl.c @@ -2321,6 +2321,11 @@ int main(int argc, char **argv, char **envp) QLIST_INIT (&vm_change_state_head); os_setup_early_signal_handling(); + if (qemu_fdlist_init() < 0) { + perror("fdlist"); + exit(EXIT_FAILURE); + } + module_call_init(MODULE_INIT_MACHINE); machine = find_default_machine(); cpu_model = NULL; -- 1.7.10.2 -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On 06/26/2012 11:37 AM, Corey Bryant wrote: Nevermind my comment. I see that your PoC supports passing nfds for any QMP command. I'm curious what Kevin's thoughts are on this and the overall approach. I'm still not quite sure how we'd go about this. We need a way to determine the existing QEMU fd that is to be re-associated with the new fd, when using a /dev/fdlist/0 filename. In this new approach, 0 corresponds to an index, not an fd. The prior approach of using the /dev/fd/nnn, where nnn corresponded to an actual QEMU fd, made this easy. -- Regards, Corey

On 06/26/2012 04:42 PM, Daniel P. Berrange wrote: The access modes (O_RDONLY, O_WRONLY, or O_RDWR) can only be set on the open() call, which is performed by libvirt. fcntl(F_SETFL) can't change them. So the point of pass-fd's force option is to allow libvirt to open() the same file with a new access mode, pass it to qemu, and copy it to the existing qemu fd. For example: fd1 = open("/path/to/file.img", O_RDONLY) fd2 = open("/path/to/file.img", O_RDWR) pass-fd disk0 (fd1) returns fd=3 (/dev/fd/3 in qemu has O_RDONLY) drive_add file:/dev/fd/3 pass-fd disk0 (fd2) returns fd=3 (/dev/fd/3 in qemu now has O_RDWR) This is required because the access_mode flags used by qemu_open() are the same as the passed fd's access_mode flags. And qemu will re-open files with different access modes. Yes, no problem and thanks again for the code. :) I'm hoping we can get some more input soon so I can get moving in one direction or another. -- Regards, Corey