[libvirt] [PATCH v4 0/4] command line fd passing using fd sets

This series adds command line file descriptor passing support via a new -add-fd option. This is a follow-on to the existing QMP fd passing support provided in the following patch series: comments.gmane.org/gmane.comp.emulators.qemu/165463 The new -add-fd option is designed to mirror the add-fd QMP option as much as possible. Corey Bryant (4): monitor: Allow add-fd to any specified fd set monitor: Enable adding an inherited fd to an fd set monitor: Prevent removing fd from set during init qemu-config: Add new -add-fd command line option monitor.c | 142 ++++++++++++++++++++++++++++++++++--------------------- monitor.h | 3 ++ qapi-schema.json | 2 +- qemu-config.c | 22 +++++++++ qemu-options.hx | 36 ++++++++++++++ vl.c | 78 ++++++++++++++++++++++++++++++ 6 files changed, 228 insertions(+), 55 deletions(-) -- 1.7.11.4

The first call to add an fd to an fd set was previously not allowed to choose the fd set ID. The ID was generated as the first available and ensuing calls could add more fds by specifying the fd set ID. This change allows users to choose the fd set ID on the first call. Signed-off-by: Corey Bryant <coreyb@linux.vnet.ibm.com> Reviewed-by: Eric Blake <eblake@redhat.com> --- v2: -This patch is new in v2 v3: -Added fdset-id bounds checking (eblake@redhat.com) -Move 'if (!mon_fdset_cur)' change to this patch (kwolf@redhat.com) -Add break from for loop if fdset_id is less than mon_fdset->id (eblake@redhat.com) v4: -No changes monitor.c | 54 +++++++++++++++++++++++++++++++++++++----------------- qapi-schema.json | 2 +- 2 files changed, 38 insertions(+), 18 deletions(-) diff --git a/monitor.c b/monitor.c index 131b325..2e3248f 100644 --- a/monitor.c +++ b/monitor.c @@ -2135,7 +2135,7 @@ AddfdInfo *qmp_add_fd(bool has_fdset_id, int64_t fdset_id, bool has_opaque, { int fd; Monitor *mon = cur_mon; - MonFdset *mon_fdset; + MonFdset *mon_fdset = NULL; MonFdsetFd *mon_fdset_fd; AddfdInfo *fdinfo; @@ -2147,34 +2147,54 @@ AddfdInfo *qmp_add_fd(bool has_fdset_id, int64_t fdset_id, bool has_opaque, if (has_fdset_id) { QLIST_FOREACH(mon_fdset, &mon_fdsets, next) { - if (mon_fdset->id == fdset_id) { + /* Break if match found or match impossible due to ordering by ID */ + if (fdset_id <= mon_fdset->id) { + if (fdset_id < mon_fdset->id) { + mon_fdset = NULL; + } break; } } - if (mon_fdset == NULL) { - error_set(errp, QERR_INVALID_PARAMETER_VALUE, "fdset-id", - "an existing fdset-id"); - goto error; - } - } else { + } + + if (mon_fdset == NULL) { 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; + if (has_fdset_id) { + if (fdset_id < 0) { + error_set(errp, QERR_INVALID_PARAMETER_VALUE, "fdset-id", + "a non-negative value"); + goto error; + } + /* Use specified fdset ID */ + QLIST_FOREACH(mon_fdset, &mon_fdsets, next) { + mon_fdset_cur = mon_fdset; + if (fdset_id < mon_fdset_cur->id) { + break; + } + } + } else { + /* 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; } - break; } mon_fdset = g_malloc0(sizeof(*mon_fdset)); - mon_fdset->id = fdset_id_prev + 1; + if (has_fdset_id) { + mon_fdset->id = fdset_id; + } else { + mon_fdset->id = fdset_id_prev + 1; + } /* The fdset list is ordered by fdset ID */ - if (mon_fdset->id == 0) { + if (!mon_fdset_cur) { 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); diff --git a/qapi-schema.json b/qapi-schema.json index f9dbdae..12cdb74 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -2611,7 +2611,7 @@ # # Returns: @AddfdInfo on success # If file descriptor was not received, FdNotSupplied -# If @fdset-id does not exist, InvalidParameterValue +# If @fdset-id is a negative value, InvalidParameterValue # # Notes: The list of fd sets is shared by all monitor connections. # -- 1.7.11.4

qmp_add_fd() gets an fd that was received over a socket with SCM_RIGHTS and adds it to an fd set. This patch adds support that will enable adding an fd that was inherited on the command line to an fd set. Note: All of the code added to monitor_fdset_add_fd(), with the exception of the error path for non-valid fdset-id, is code motion from qmp_add_fd(). Signed-off-by: Corey Bryant <coreyb@linux.vnet.ibm.com> Reviewed-by: Eric Blake <eblake@redhat.com> --- v2: -Removed Error** parameter from monitor_fdset_add_fd() v3: -Added Error** parameter back to monitor_fdset_add_fd() -Move 'if (!mon_fdset_cur)' change to patch 1 (kwolf@redhat.com) -Move code that prevents removal of fd from fd set during init to it's own patch (eblake@redhat.com, kwolf@redhat.com) -Mention code motion in commit message (kwolf@redhat.com) v4: -Fix fd leak in qmp_add_fd() (kwolf@redhat.com) monitor.c | 157 ++++++++++++++++++++++++++++++++++---------------------------- monitor.h | 3 ++ 2 files changed, 88 insertions(+), 72 deletions(-) diff --git a/monitor.c b/monitor.c index 2e3248f..d8f66ca 100644 --- a/monitor.c +++ b/monitor.c @@ -2135,8 +2135,6 @@ AddfdInfo *qmp_add_fd(bool has_fdset_id, int64_t fdset_id, bool has_opaque, { int fd; Monitor *mon = cur_mon; - MonFdset *mon_fdset = NULL; - MonFdsetFd *mon_fdset_fd; AddfdInfo *fdinfo; fd = qemu_chr_fe_get_msgfd(mon->chr); @@ -2145,78 +2143,12 @@ AddfdInfo *qmp_add_fd(bool has_fdset_id, int64_t fdset_id, bool has_opaque, goto error; } - if (has_fdset_id) { - QLIST_FOREACH(mon_fdset, &mon_fdsets, next) { - /* Break if match found or match impossible due to ordering by ID */ - if (fdset_id <= mon_fdset->id) { - if (fdset_id < mon_fdset->id) { - mon_fdset = NULL; - } - break; - } - } + fdinfo = monitor_fdset_add_fd(fd, has_fdset_id, fdset_id, + has_opaque, opaque, errp); + if (fdinfo) { + return fdinfo; } - if (mon_fdset == NULL) { - int64_t fdset_id_prev = -1; - MonFdset *mon_fdset_cur = QLIST_FIRST(&mon_fdsets); - - if (has_fdset_id) { - if (fdset_id < 0) { - error_set(errp, QERR_INVALID_PARAMETER_VALUE, "fdset-id", - "a non-negative value"); - goto error; - } - /* Use specified fdset ID */ - QLIST_FOREACH(mon_fdset, &mon_fdsets, next) { - mon_fdset_cur = mon_fdset; - if (fdset_id < mon_fdset_cur->id) { - break; - } - } - } else { - /* 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)); - if (has_fdset_id) { - mon_fdset->id = fdset_id; - } else { - mon_fdset->id = fdset_id_prev + 1; - } - - /* The fdset list is ordered by fdset ID */ - if (!mon_fdset_cur) { - 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); @@ -2301,6 +2233,87 @@ FdsetInfoList *qmp_query_fdsets(Error **errp) return fdset_list; } +AddfdInfo *monitor_fdset_add_fd(int fd, bool has_fdset_id, int64_t fdset_id, + bool has_opaque, const char *opaque, + Error **errp) +{ + MonFdset *mon_fdset = NULL; + MonFdsetFd *mon_fdset_fd; + AddfdInfo *fdinfo; + + if (has_fdset_id) { + QLIST_FOREACH(mon_fdset, &mon_fdsets, next) { + /* Break if match found or match impossible due to ordering by ID */ + if (fdset_id <= mon_fdset->id) { + if (fdset_id < mon_fdset->id) { + mon_fdset = NULL; + } + break; + } + } + } + + if (mon_fdset == NULL) { + int64_t fdset_id_prev = -1; + MonFdset *mon_fdset_cur = QLIST_FIRST(&mon_fdsets); + + if (has_fdset_id) { + if (fdset_id < 0) { + error_set(errp, QERR_INVALID_PARAMETER_VALUE, "fdset-id", + "a non-negative value"); + return NULL; + } + /* Use specified fdset ID */ + QLIST_FOREACH(mon_fdset, &mon_fdsets, next) { + mon_fdset_cur = mon_fdset; + if (fdset_id < mon_fdset_cur->id) { + break; + } + } + } else { + /* 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)); + if (has_fdset_id) { + mon_fdset->id = fdset_id; + } else { + mon_fdset->id = fdset_id_prev + 1; + } + + /* The fdset list is ordered by fdset ID */ + if (!mon_fdset_cur) { + 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; +} + int monitor_fdset_get_fd(int64_t fdset_id, int flags) { #ifndef _WIN32 diff --git a/monitor.h b/monitor.h index b6e7d95..d4c017e 100644 --- a/monitor.h +++ b/monitor.h @@ -90,6 +90,9 @@ int qmp_qom_set(Monitor *mon, const QDict *qdict, QObject **ret); int qmp_qom_get(Monitor *mon, const QDict *qdict, QObject **ret); +AddfdInfo *monitor_fdset_add_fd(int fd, bool has_fdset_id, int64_t fdset_id, + bool has_opaque, const char *opaque, + Error **errp); int monitor_fdset_get_fd(int64_t fdset_id, int flags); int monitor_fdset_dup_fd_add(int64_t fdset_id, int dup_fd); int monitor_fdset_dup_fd_remove(int dup_fd); -- 1.7.11.4

If an fd is added to an fd set via the command line, and it is not referenced by another command line option (ie. -drive), then clean it up after QEMU initialization is complete. Signed-off-by: Corey Bryant <coreyb@linux.vnet.ibm.com> Reviewed-by: Eric Blake <eblake@redhat.com> --- v3: - This patch was split into it's own patch in v3 (eblake@redhat.com, kwolf@redhat.com) v4: - No changes monitor.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/monitor.c b/monitor.c index d8f66ca..928e3ae 100644 --- a/monitor.c +++ b/monitor.c @@ -2105,8 +2105,9 @@ 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 || - (QLIST_EMPTY(&mon_fdset->dup_fds) && mon_refcount == 0)) { + if ((mon_fdset_fd->removed || + (QLIST_EMPTY(&mon_fdset->dup_fds) && mon_refcount == 0)) && + runstate_is_running()) { close(mon_fdset_fd->fd); g_free(mon_fdset_fd->opaque); QLIST_REMOVE(mon_fdset_fd, next); -- 1.7.11.4

This option can be used for passing file descriptors on the command line. It mirrors the existing add-fd QMP command which allows an fd to be passed to QEMU via SCM_RIGHTS and added to an fd set. This can be combined with commands such as -drive to link file descriptors in an fd set to a drive: qemu-kvm -add-fd fd=3,set=2,opaque="rdwr:/path/to/file" -add-fd fd=4,set=2,opaque="rdonly:/path/to/file" -drive file=/dev/fdset/2,index=0,media=disk This example adds dups of fds 3 and 4, and the accompanying opaque strings to the fd set with ID=2. qemu_open() already knows how to handle a filename of this format. qemu_open() searches the corresponding fd set for an fd and when it finds a match, QEMU goes on to use a dup of that fd just like it would have used an fd that it opened itself. Signed-off-by: Corey Bryant <coreyb@linux.vnet.ibm.com> --- v2: - The -add-fd option is new in v2 (eblake@redhat.com) v3: - Require passed fd to be > stderr (eblake@redhat.com) - Changed fds in examples to fd=3 and fd=4 - Add dup of passed fd to fd set and close passed fds after processing all -add-fd commands (eblake@redhat.com) v4 - Update fd numbers in commit message text (eblake@redhat.com) - Handle corner case of -add-fd using an fd that is already in use by QEMU (eblake@redhat.com) qemu-config.c | 22 ++++++++++++++++ qemu-options.hx | 36 ++++++++++++++++++++++++++ vl.c | 78 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 136 insertions(+) diff --git a/qemu-config.c b/qemu-config.c index cd1ec21..601237d 100644 --- a/qemu-config.c +++ b/qemu-config.c @@ -653,6 +653,27 @@ QemuOptsList qemu_boot_opts = { }, }; +static QemuOptsList qemu_add_fd_opts = { + .name = "add-fd", + .head = QTAILQ_HEAD_INITIALIZER(qemu_add_fd_opts.head), + .desc = { + { + .name = "fd", + .type = QEMU_OPT_NUMBER, + .help = "file descriptor of which a duplicate is added to fd set", + },{ + .name = "set", + .type = QEMU_OPT_NUMBER, + .help = "ID of the fd set to add fd to", + },{ + .name = "opaque", + .type = QEMU_OPT_STRING, + .help = "free-form string used to describe fd", + }, + { /* end of list */ } + }, +}; + static QemuOptsList *vm_config_groups[32] = { &qemu_drive_opts, &qemu_chardev_opts, @@ -669,6 +690,7 @@ static QemuOptsList *vm_config_groups[32] = { &qemu_boot_opts, &qemu_iscsi_opts, &qemu_sandbox_opts, + &qemu_add_fd_opts, NULL, }; diff --git a/qemu-options.hx b/qemu-options.hx index 7d97f96..a70182a 100644 --- a/qemu-options.hx +++ b/qemu-options.hx @@ -257,6 +257,14 @@ qemu-system-i386 -drive file=file,index=2,media=disk qemu-system-i386 -drive file=file,index=3,media=disk @end example +You can open an image using pre-opened file descriptors from an fd set: +@example +qemu-system-i386 +-add-fd fd=3,set=2,opaque="rdwr:/path/to/file" +-add-fd fd=4,set=2,opaque="rdonly:/path/to/file" +-drive file=/dev/fdset/2,index=0,media=disk +@end example + You can connect a CDROM to the slave of ide0: @example qemu-system-i386 -drive file=file,if=ide,index=1,media=cdrom @@ -289,6 +297,34 @@ qemu-system-i386 -hda a -hdb b @end example ETEXI +DEF("add-fd", HAS_ARG, QEMU_OPTION_add_fd, + "-add-fd fd=fd,set=set[,opaque=opaque]\n" + " Add 'fd' to fd 'set'\n", QEMU_ARCH_ALL) +STEXI +@item -add-fd fd=@var{fd},set=@var{set}[,opaque=@var{opaque}] +@findex -add-fd + +Add a file descriptor to an fd set. Valid options are: + +@table @option +@item fd=@var{fd} +This option defines the file descriptor of which a duplicate is added to fd set. +The file descriptor cannot be stdin, stdout, or stderr. +@item set=@var{set} +This option defines the ID of the fd set to add the file descriptor to. +@item opaque=@var{opaque} +This option defines a free-form string that can be used to describe @var{fd}. +@end table + +You can open an image using pre-opened file descriptors from an fd set: +@example +qemu-system-i386 +-add-fd fd=3,set=2,opaque="rdwr:/path/to/file" +-add-fd fd=4,set=2,opaque="rdonly:/path/to/file" +-drive file=/dev/fdset/2,index=0,media=disk +@end example +ETEXI + DEF("set", HAS_ARG, QEMU_OPTION_set, "-set group.id.arg=value\n" " set <arg> parameter for item <id> of type <group>\n" diff --git a/vl.c b/vl.c index 5b357a3..47095a2 100644 --- a/vl.c +++ b/vl.c @@ -790,6 +790,71 @@ static int parse_sandbox(QemuOpts *opts, void *opaque) return 0; } +static int parse_add_fd(QemuOpts *opts, void *opaque) +{ + int fd, dupfd; + int64_t fdset_id; + const char *fd_opaque = NULL; + + fd = qemu_opt_get_number(opts, "fd", -1); + fdset_id = qemu_opt_get_number(opts, "set", -1); + fd_opaque = qemu_opt_get(opts, "opaque"); + + if (fd < 0) { + qerror_report(ERROR_CLASS_GENERIC_ERROR, + "fd option is required and must be non-negative"); + return -1; + } + + if (fd <= STDERR_FILENO) { + qerror_report(ERROR_CLASS_GENERIC_ERROR, + "fd cannot be a standard I/O stream"); + return -1; + } + + if (fcntl(fd, F_GETFD) & FD_CLOEXEC) { + qerror_report(ERROR_CLASS_GENERIC_ERROR, + "fd is not valid or already in use"); + return -1; + } + + if (fdset_id < 0) { + qerror_report(ERROR_CLASS_GENERIC_ERROR, + "set option is required and must be non-negative"); + return -1; + } + +#ifdef F_DUPFD_CLOEXEC + dupfd = fcntl(fd, F_DUPFD_CLOEXEC, 0); +#else + dupfd = dup(fd); + if (dupfd != -1) { + qemu_set_cloexec(dupfd); + } +#endif + if (dupfd == -1) { + qerror_report(ERROR_CLASS_GENERIC_ERROR, + "Error duplicating fd: %s", strerror(errno)); + return -1; + } + + /* add the duplicate fd, and optionally the opaque string, to the fd set */ + monitor_fdset_add_fd(dupfd, true, fdset_id, fd_opaque ? true : false, + fd_opaque, NULL); + + return 0; +} + +static int cleanup_add_fd(QemuOpts *opts, void *opaque) +{ + int fd; + + fd = qemu_opt_get_number(opts, "fd", -1); + close(fd); + + return 0; +} + /***********************************************************/ /* QEMU Block devices */ @@ -3309,6 +3374,11 @@ int main(int argc, char **argv, char **envp) exit(0); } break; + case QEMU_OPTION_add_fd: + opts = qemu_opts_parse(qemu_find_opts("add-fd"), optarg, 0); + if (!opts) { + exit(0); + } default: os_parse_cmd_args(popt->index, optarg); } @@ -3320,6 +3390,14 @@ int main(int argc, char **argv, char **envp) exit(1); } + if (qemu_opts_foreach(qemu_find_opts("add-fd"), parse_add_fd, NULL, 1)) { + exit(1); + } + + if (qemu_opts_foreach(qemu_find_opts("add-fd"), cleanup_add_fd, NULL, 1)) { + exit(1); + } + if (machine == NULL) { fprintf(stderr, "No machine found.\n"); exit(1); -- 1.7.11.4

On 10/18/2012 01:19 PM, Corey Bryant wrote:
This option can be used for passing file descriptors on the command line. It mirrors the existing add-fd QMP command which allows an fd to be passed to QEMU via SCM_RIGHTS and added to an fd set.
This can be combined with commands such as -drive to link file descriptors in an fd set to a drive:
qemu-kvm -add-fd fd=3,set=2,opaque="rdwr:/path/to/file" -add-fd fd=4,set=2,opaque="rdonly:/path/to/file" -drive file=/dev/fdset/2,index=0,media=disk
This example adds dups of fds 3 and 4, and the accompanying opaque strings to the fd set with ID=2. qemu_open() already knows how to handle a filename of this format. qemu_open() searches the corresponding fd set for an fd and when it finds a match, QEMU goes on to use a dup of that fd just like it would have used an fd that it opened itself.
Signed-off-by: Corey Bryant <coreyb@linux.vnet.ibm.com>
+ + if (fcntl(fd, F_GETFD) & FD_CLOEXEC) { + qerror_report(ERROR_CLASS_GENERIC_ERROR, + "fd is not valid or already in use"); + return -1; + }
Hmm, I was about to call you on the fact that you didn't check whether fcntl() succeeded; but then realized that in the failure case it is required by POSIX to return -1 which happens to include the FD_CLOEXEC bit, so you actually ended up with a sneaky optimization that does the right thing for both open and closed fds. Perhaps a comment in the code is warranted (after all, it is not immediately apparent from reading just this if statement why it works); maybe "/* All fds inherited across exec() necessarily have FD_CLOEXEC clear, while qemu sets FD_CLOEXEC on all other fds opened from command line arguments */". But I'm not going to require a v5 just for a comment addition. Series: Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 10/18/2012 04:43 PM, Eric Blake wrote:
On 10/18/2012 01:19 PM, Corey Bryant wrote:
This option can be used for passing file descriptors on the command line. It mirrors the existing add-fd QMP command which allows an fd to be passed to QEMU via SCM_RIGHTS and added to an fd set.
This can be combined with commands such as -drive to link file descriptors in an fd set to a drive:
qemu-kvm -add-fd fd=3,set=2,opaque="rdwr:/path/to/file" -add-fd fd=4,set=2,opaque="rdonly:/path/to/file" -drive file=/dev/fdset/2,index=0,media=disk
This example adds dups of fds 3 and 4, and the accompanying opaque strings to the fd set with ID=2. qemu_open() already knows how to handle a filename of this format. qemu_open() searches the corresponding fd set for an fd and when it finds a match, QEMU goes on to use a dup of that fd just like it would have used an fd that it opened itself.
Signed-off-by: Corey Bryant <coreyb@linux.vnet.ibm.com>
+ + if (fcntl(fd, F_GETFD) & FD_CLOEXEC) { + qerror_report(ERROR_CLASS_GENERIC_ERROR, + "fd is not valid or already in use"); + return -1; + }
Hmm, I was about to call you on the fact that you didn't check whether fcntl() succeeded; but then realized that in the failure case it is required by POSIX to return -1 which happens to include the FD_CLOEXEC bit, so you actually ended up with a sneaky optimization that does the right thing for both open and closed fds.
Yep it works for both cases. I have to admit I stumbled into this at first and then decided to leave it this way since it worked. :)
Perhaps a comment in the code is warranted (after all, it is not immediately apparent from reading just this if statement why it works); maybe "/* All fds inherited across exec() necessarily have FD_CLOEXEC clear, while qemu sets FD_CLOEXEC on all other fds opened from command line arguments */". But I'm not going to require a v5 just for a comment addition.
I agree, a comment would be useful. Maybe Kevin can add if this series gets pushed?
Series: Reviewed-by: Eric Blake <eblake@redhat.com>
Thanks for reviewing! -- Regards, Corey Bryant

Am 18.10.2012 23:37, schrieb Corey Bryant:
On 10/18/2012 04:43 PM, Eric Blake wrote:
On 10/18/2012 01:19 PM, Corey Bryant wrote:
This option can be used for passing file descriptors on the command line. It mirrors the existing add-fd QMP command which allows an fd to be passed to QEMU via SCM_RIGHTS and added to an fd set.
This can be combined with commands such as -drive to link file descriptors in an fd set to a drive:
qemu-kvm -add-fd fd=3,set=2,opaque="rdwr:/path/to/file" -add-fd fd=4,set=2,opaque="rdonly:/path/to/file" -drive file=/dev/fdset/2,index=0,media=disk
This example adds dups of fds 3 and 4, and the accompanying opaque strings to the fd set with ID=2. qemu_open() already knows how to handle a filename of this format. qemu_open() searches the corresponding fd set for an fd and when it finds a match, QEMU goes on to use a dup of that fd just like it would have used an fd that it opened itself.
Signed-off-by: Corey Bryant <coreyb@linux.vnet.ibm.com>
+ + if (fcntl(fd, F_GETFD) & FD_CLOEXEC) { + qerror_report(ERROR_CLASS_GENERIC_ERROR, + "fd is not valid or already in use"); + return -1; + }
Hmm, I was about to call you on the fact that you didn't check whether fcntl() succeeded; but then realized that in the failure case it is required by POSIX to return -1 which happens to include the FD_CLOEXEC bit, so you actually ended up with a sneaky optimization that does the right thing for both open and closed fds.
Yep it works for both cases. I have to admit I stumbled into this at first and then decided to leave it this way since it worked. :)
I wouldn't be surprised to find such subtleties in Fabrice's code, but I'm not sure if adding new instances is the best idea ever. :-)
Perhaps a comment in the code is warranted (after all, it is not immediately apparent from reading just this if statement why it works); maybe "/* All fds inherited across exec() necessarily have FD_CLOEXEC clear, while qemu sets FD_CLOEXEC on all other fds opened from command line arguments */". But I'm not going to require a v5 just for a comment addition.
I agree, a comment would be useful. Maybe Kevin can add if this series gets pushed?
I'll amend the following to this patch, hope you both agree with the change: diff --git a/vl.c b/vl.c index 47095a2..5fb40da 100644 --- a/vl.c +++ b/vl.c @@ -792,7 +792,7 @@ static int parse_sandbox(QemuOpts *opts, void *opaque) static int parse_add_fd(QemuOpts *opts, void *opaque) { - int fd, dupfd; + int fd, dupfd, flags; int64_t fdset_id; const char *fd_opaque = NULL; @@ -812,7 +812,12 @@ static int parse_add_fd(QemuOpts *opts, void *opaque) return -1; } - if (fcntl(fd, F_GETFD) & FD_CLOEXEC) { + /* + * All fds inherited across exec() necessarily have FD_CLOEXEC + * clear, while qemu sets FD_CLOEXEC on all other fds used internally. + */ + flags = fcntl(fd, F_GETFD); + if (flags == -1 || (flags & FD_CLOEXEC)) { qerror_report(ERROR_CLASS_GENERIC_ERROR, "fd is not valid or already in use"); return -1;

On 10/19/2012 07:05 AM, Kevin Wolf wrote:
Am 18.10.2012 23:37, schrieb Corey Bryant:
On 10/18/2012 04:43 PM, Eric Blake wrote:
On 10/18/2012 01:19 PM, Corey Bryant wrote:
This option can be used for passing file descriptors on the command line. It mirrors the existing add-fd QMP command which allows an fd to be passed to QEMU via SCM_RIGHTS and added to an fd set.
This can be combined with commands such as -drive to link file descriptors in an fd set to a drive:
qemu-kvm -add-fd fd=3,set=2,opaque="rdwr:/path/to/file" -add-fd fd=4,set=2,opaque="rdonly:/path/to/file" -drive file=/dev/fdset/2,index=0,media=disk
This example adds dups of fds 3 and 4, and the accompanying opaque strings to the fd set with ID=2. qemu_open() already knows how to handle a filename of this format. qemu_open() searches the corresponding fd set for an fd and when it finds a match, QEMU goes on to use a dup of that fd just like it would have used an fd that it opened itself.
Signed-off-by: Corey Bryant <coreyb@linux.vnet.ibm.com>
+ + if (fcntl(fd, F_GETFD) & FD_CLOEXEC) { + qerror_report(ERROR_CLASS_GENERIC_ERROR, + "fd is not valid or already in use"); + return -1; + }
Hmm, I was about to call you on the fact that you didn't check whether fcntl() succeeded; but then realized that in the failure case it is required by POSIX to return -1 which happens to include the FD_CLOEXEC bit, so you actually ended up with a sneaky optimization that does the right thing for both open and closed fds.
Yep it works for both cases. I have to admit I stumbled into this at first and then decided to leave it this way since it worked. :)
I wouldn't be surprised to find such subtleties in Fabrice's code, but I'm not sure if adding new instances is the best idea ever. :-)
Perhaps a comment in the code is warranted (after all, it is not immediately apparent from reading just this if statement why it works); maybe "/* All fds inherited across exec() necessarily have FD_CLOEXEC clear, while qemu sets FD_CLOEXEC on all other fds opened from command line arguments */". But I'm not going to require a v5 just for a comment addition.
I agree, a comment would be useful. Maybe Kevin can add if this series gets pushed?
I'll amend the following to this patch, hope you both agree with the change:
diff --git a/vl.c b/vl.c index 47095a2..5fb40da 100644 --- a/vl.c +++ b/vl.c @@ -792,7 +792,7 @@ static int parse_sandbox(QemuOpts *opts, void *opaque)
static int parse_add_fd(QemuOpts *opts, void *opaque) { - int fd, dupfd; + int fd, dupfd, flags; int64_t fdset_id; const char *fd_opaque = NULL;
@@ -812,7 +812,12 @@ static int parse_add_fd(QemuOpts *opts, void *opaque) return -1; }
- if (fcntl(fd, F_GETFD) & FD_CLOEXEC) { + /* + * All fds inherited across exec() necessarily have FD_CLOEXEC + * clear, while qemu sets FD_CLOEXEC on all other fds used internally. + */ + flags = fcntl(fd, F_GETFD); + if (flags == -1 || (flags & FD_CLOEXEC)) { qerror_report(ERROR_CLASS_GENERIC_ERROR, "fd is not valid or already in use"); return -1;
That works for me. Thanks Kevin! -- Regards, Corey Bryant

On 10/18/2012 01:19 PM, Corey Bryant wrote:
This option can be used for passing file descriptors on the command line. It mirrors the existing add-fd QMP command which allows an fd to be passed to QEMU via SCM_RIGHTS and added to an fd set.
+ +static int cleanup_add_fd(QemuOpts *opts, void *opaque) +{ + int fd; + + fd = qemu_opt_get_number(opts, "fd", -1); + close(fd);
One other subtle point: Given 'qemu-kvm -add-fd fd=3,set=1 -add-fd fd=3,set=2', this code will call close(3) twice. In a single-threaded scenario, we happen to be safe (because we merely ignore that the second one fails with EBADF), but in a multi-threaded scenario, it is a recipe for disaster with a chance for closing an fd opened by another thread.
@@ -3320,6 +3390,14 @@ int main(int argc, char **argv, char **envp) exit(1); }
+ if (qemu_opts_foreach(qemu_find_opts("add-fd"), parse_add_fd, NULL, 1)) { + exit(1); + } + + if (qemu_opts_foreach(qemu_find_opts("add-fd"), cleanup_add_fd, NULL, 1)) { + exit(1); + }
Thankfully, we only ever call that code in main(), prior to spawning threads. So my positive review still stands. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (3)
-
Corey Bryant
-
Eric Blake
-
Kevin Wolf