| Message ID | 1350411046-2453-1-git-send-email-coreyb@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
On 10/16/2012 12:10 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 4 and 5, and the accompanying opaque s/4 and 5/3 and 4/ > 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) I'm still seeing the corner case of: qemu-kvm -add-fd fd=3,set=1 -add-fd fd=4,set=2 4<&- where the dup(3) will populate fd 4 prior to the point where we get to process the -add-fd fd=4 command to notice that the user started qemu-kvm with fd 4 closed, and thus qemu will silently proceed to use the wrong fd. On the other hand, I'm not sure if that corner case is worth worrying about, or if we just chalk it up to user stupidity (aka libvirt programmer stupidity) if they did something like that (most likely, because the management app forgot to clear FD_CLOEXEC before exec()ing qemu-kvm). Hmm, this makes me wonder if I can do something crazy like: qemu-kvm -add-fd fd=4,set=1 -qmp /dev/fdset/1 to open a monitor on the fd I just passed in? And what if so, what then happens on that monitor if I request that fdset 1 be removed?
Am 17.10.2012 06:16, schrieb Eric Blake: > I'm still seeing the corner case of: > > qemu-kvm -add-fd fd=3,set=1 -add-fd fd=4,set=2 4<&- > > where the dup(3) will populate fd 4 prior to the point where we get to > process the -add-fd fd=4 command to notice that the user started > qemu-kvm with fd 4 closed, and thus qemu will silently proceed to use > the wrong fd. > > On the other hand, I'm not sure if that corner case is worth worrying > about, or if we just chalk it up to user stupidity (aka libvirt > programmer stupidity) if they did something like that (most likely, > because the management app forgot to clear FD_CLOEXEC before exec()ing > qemu-kvm). If you specify an FD number that isn't actually open when qemu is stared, you can get any FD that qemu opens internally. I think the correct answer to this problem is "then don't do that". > Hmm, this makes me wonder if I can do something crazy like: > > qemu-kvm -add-fd fd=4,set=1 -qmp /dev/fdset/1 > > to open a monitor on the fd I just passed in? I think so. At least on my side it was intended to allow this. > And what if so, what then > happens on that monitor if I request that fdset 1 be removed? The same as with block devices: The fd stays open until the monitor connection is closed. A closed monitor also triggers fd garbage collection, so at this point the original fd would be closed (well, assuming that you had only one monitor). Kevin
On 10/17/2012 08:02 AM, Kevin Wolf wrote: > Am 17.10.2012 06:16, schrieb Eric Blake: >> I'm still seeing the corner case of: >> >> qemu-kvm -add-fd fd=3,set=1 -add-fd fd=4,set=2 4<&- >> >> where the dup(3) will populate fd 4 prior to the point where we get to >> process the -add-fd fd=4 command to notice that the user started >> qemu-kvm with fd 4 closed, and thus qemu will silently proceed to use >> the wrong fd. >> >> On the other hand, I'm not sure if that corner case is worth worrying >> about, or if we just chalk it up to user stupidity (aka libvirt >> programmer stupidity) if they did something like that (most likely, >> because the management app forgot to clear FD_CLOEXEC before exec()ing >> qemu-kvm). > > If you specify an FD number that isn't actually open when qemu is > stared, you can get any FD that qemu opens internally. I think the > correct answer to this problem is "then don't do that". Overnight, I realized we do have one potential safety valve: we are guaranteed that any fd inherited by the exec() of qemu-kvm has FD_CLOEXEC clear, and we also strive to have qemu open/dup all of its internal fds with FD_CLOEXEC set. Therefore, it may be worth a sanity check of fcntl(F_GETFD) to see if FD_CLOEXEC is set, and if so, the user must have failed to pass in the fd, and we are now looking at a qemu internal fd, and should therefore report failure. But I'm not sure if it's worth the extra code.
Am 17.10.2012 17:01, schrieb Eric Blake: > On 10/17/2012 08:02 AM, Kevin Wolf wrote: >> Am 17.10.2012 06:16, schrieb Eric Blake: >>> I'm still seeing the corner case of: >>> >>> qemu-kvm -add-fd fd=3,set=1 -add-fd fd=4,set=2 4<&- >>> >>> where the dup(3) will populate fd 4 prior to the point where we get to >>> process the -add-fd fd=4 command to notice that the user started >>> qemu-kvm with fd 4 closed, and thus qemu will silently proceed to use >>> the wrong fd. >>> >>> On the other hand, I'm not sure if that corner case is worth worrying >>> about, or if we just chalk it up to user stupidity (aka libvirt >>> programmer stupidity) if they did something like that (most likely, >>> because the management app forgot to clear FD_CLOEXEC before exec()ing >>> qemu-kvm). >> >> If you specify an FD number that isn't actually open when qemu is >> stared, you can get any FD that qemu opens internally. I think the >> correct answer to this problem is "then don't do that". > > Overnight, I realized we do have one potential safety valve: we are > guaranteed that any fd inherited by the exec() of qemu-kvm has > FD_CLOEXEC clear, and we also strive to have qemu open/dup all of its > internal fds with FD_CLOEXEC set. Therefore, it may be worth a sanity > check of fcntl(F_GETFD) to see if FD_CLOEXEC is set, and if so, the user > must have failed to pass in the fd, and we are now looking at a qemu > internal fd, and should therefore report failure. But I'm not sure if > it's worth the extra code. Hm, this sounds actually easy enough. I'll leave the decision to Corey, but I like the idea. Kevin
On 10/17/2012 12:16 AM, Eric Blake wrote: > On 10/16/2012 12:10 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 4 and 5, and the accompanying opaque > > s/4 and 5/3 and 4/ Doh.. thanks.
On 10/17/2012 10:02 AM, Kevin Wolf wrote: > Am 17.10.2012 06:16, schrieb Eric Blake: >> I'm still seeing the corner case of: >> >> qemu-kvm -add-fd fd=3,set=1 -add-fd fd=4,set=2 4<&- >> >> where the dup(3) will populate fd 4 prior to the point where we get to >> process the -add-fd fd=4 command to notice that the user started >> qemu-kvm with fd 4 closed, and thus qemu will silently proceed to use >> the wrong fd. >> >> On the other hand, I'm not sure if that corner case is worth worrying >> about, or if we just chalk it up to user stupidity (aka libvirt >> programmer stupidity) if they did something like that (most likely, >> because the management app forgot to clear FD_CLOEXEC before exec()ing >> qemu-kvm). > > If you specify an FD number that isn't actually open when qemu is > stared, you can get any FD that qemu opens internally. I think the > correct answer to this problem is "then don't do that". > I'd also say "then don't do that". Or maybe "why are you doing that?". But I'm not opposed to closing a corner case if it's not cluttering the code base. >> Hmm, this makes me wonder if I can do something crazy like: >> >> qemu-kvm -add-fd fd=4,set=1 -qmp /dev/fdset/1 >> >> to open a monitor on the fd I just passed in? > > I think so. At least on my side it was intended to allow this. > >> And what if so, what then >> happens on that monitor if I request that fdset 1 be removed? > > The same as with block devices: The fd stays open until the monitor > connection is closed. A closed monitor also triggers fd garbage > collection, so at this point the original fd would be closed (well, > assuming that you had only one monitor). > > Kevin > True, but I think in this case we care more about the dup'd fd staying open than the fd in the fdset. Remember that qemu_open() dups the fd from the fd set. So assuming the open/close of the QMP fd occurs in qemu_open()/qemu_close(), the QMP fd would be a dup of the fd that was added to the fd set. So if remove-fd removed the fd from the fdset, or it removed the entire fdset, the QMP fd would remain open until qemu_close() was called. I'll try this out today to make sure but I don't think this is an issue.
On 10/17/2012 11:03 AM, Kevin Wolf wrote: > Am 17.10.2012 17:01, schrieb Eric Blake: >> On 10/17/2012 08:02 AM, Kevin Wolf wrote: >>> Am 17.10.2012 06:16, schrieb Eric Blake: >>>> I'm still seeing the corner case of: >>>> >>>> qemu-kvm -add-fd fd=3,set=1 -add-fd fd=4,set=2 4<&- >>>> >>>> where the dup(3) will populate fd 4 prior to the point where we get to >>>> process the -add-fd fd=4 command to notice that the user started >>>> qemu-kvm with fd 4 closed, and thus qemu will silently proceed to use >>>> the wrong fd. >>>> >>>> On the other hand, I'm not sure if that corner case is worth worrying >>>> about, or if we just chalk it up to user stupidity (aka libvirt >>>> programmer stupidity) if they did something like that (most likely, >>>> because the management app forgot to clear FD_CLOEXEC before exec()ing >>>> qemu-kvm). >>> >>> If you specify an FD number that isn't actually open when qemu is >>> stared, you can get any FD that qemu opens internally. I think the >>> correct answer to this problem is "then don't do that". >> >> Overnight, I realized we do have one potential safety valve: we are >> guaranteed that any fd inherited by the exec() of qemu-kvm has >> FD_CLOEXEC clear, and we also strive to have qemu open/dup all of its >> internal fds with FD_CLOEXEC set. Therefore, it may be worth a sanity >> check of fcntl(F_GETFD) to see if FD_CLOEXEC is set, and if so, the user >> must have failed to pass in the fd, and we are now looking at a qemu >> internal fd, and should therefore report failure. But I'm not sure if >> it's worth the extra code. > > Hm, this sounds actually easy enough. I'll leave the decision to Corey, > but I like the idea. Sure I can add this.
On 10/18/2012 10:29 AM, Corey Bryant wrote: > > > On 10/17/2012 10:02 AM, Kevin Wolf wrote: >> Am 17.10.2012 06:16, schrieb Eric Blake: >>> I'm still seeing the corner case of: >>> >>> qemu-kvm -add-fd fd=3,set=1 -add-fd fd=4,set=2 4<&- >>> >>> where the dup(3) will populate fd 4 prior to the point where we get to >>> process the -add-fd fd=4 command to notice that the user started >>> qemu-kvm with fd 4 closed, and thus qemu will silently proceed to use >>> the wrong fd. >>> >>> On the other hand, I'm not sure if that corner case is worth worrying >>> about, or if we just chalk it up to user stupidity (aka libvirt >>> programmer stupidity) if they did something like that (most likely, >>> because the management app forgot to clear FD_CLOEXEC before exec()ing >>> qemu-kvm). >> >> If you specify an FD number that isn't actually open when qemu is >> stared, you can get any FD that qemu opens internally. I think the >> correct answer to this problem is "then don't do that". >> > > I'd also say "then don't do that". Or maybe "why are you doing that?". > But I'm not opposed to closing a corner case if it's not cluttering the > code base. > >>> Hmm, this makes me wonder if I can do something crazy like: >>> >>> qemu-kvm -add-fd fd=4,set=1 -qmp /dev/fdset/1 >>> >>> to open a monitor on the fd I just passed in? >> >> I think so. At least on my side it was intended to allow this. >> >>> And what if so, what then >>> happens on that monitor if I request that fdset 1 be removed? >> >> The same as with block devices: The fd stays open until the monitor >> connection is closed. A closed monitor also triggers fd garbage >> collection, so at this point the original fd would be closed (well, >> assuming that you had only one monitor). >> >> Kevin >> > > True, but I think in this case we care more about the dup'd fd staying > open than the fd in the fdset. Remember that qemu_open() dups the fd > from the fd set. So assuming the open/close of the QMP fd occurs in > qemu_open()/qemu_close(), the QMP fd would be a dup of the fd that was > added to the fd set. So if remove-fd removed the fd from the fdset, or > it removed the entire fdset, the QMP fd would remain open until > qemu_close() was called. I'll try this out today to make sure but I > don't think this is an issue. > After digging into this some more it appears to be a non issue. Only qemu_open() and qemu_close() deal with fdsets. The QMP fd is created with qemu_socket(), not qemu_open(), so it doesn't deal with fdsets. The ensuing bind() call that specifies the path ends up failing with ENOENT because the actual path "/dev/fdset/1" doesn't exist: bind(unix:/dev/fdset/1): No such file or directory
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..83205c0 100644 --- a/vl.c +++ b/vl.c @@ -790,6 +790,65 @@ 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 (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 +3368,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 +3384,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 4 and 5, 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) qemu-config.c | 22 ++++++++++++++++++ qemu-options.hx | 36 +++++++++++++++++++++++++++++ vl.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 130 insertions(+)