Message ID | 1350587974-6378-5-git-send-email-coreyb@linux.vnet.ibm.com |
---|---|
State | New |
Headers | show |
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>
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!
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.
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);
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(+)