| Message ID | 1349878805-16352-4-git-send-email-coreyb@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
On 10/10/2012 08:20 AM, 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=4,set=2,opaque="rdwr:/path/to/file" > -add-fd fd=5,set=2,opaque="rdonly:/path/to/file" > -drive file=/dev/fdset/2,index=0,media=disk > > This example adds 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. Missing some argument validation. Earlier, I complained about set validation, now I'm going to complain about fd validation. > +static int parse_add_fd(QemuOpts *opts, void *opaque) > +{ > + int fd; > + 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 I call 'qemu -add-fd fd=-2,set=1', that had better fail because fd -2 does not exist. Likewise if I call 'qemu -add-fd fd=10000000,set=1' (here, picking an fd that I know can't be opened). More subtly, 'qemu -add-fd fd=4,set=1 4<&-' should fail, since fd 4 was not inherited down to the qemu process. Fuzzier is whether 'qemu -add-fd fd=0,set=1' makes sense, as that would then make stdin be competing for multiple uses; this would be a situation that the monitor command can't duplicate, so you have introduced new ways to possibly break things from the command line. I'm thinking it's safest to reject fd of 0, 1, or 2 for now, and only relax it later IF we can prove that it would be both useful and safe. So it sounds like you need something like fcntl(fd,F_GETFL) to see that an the fd was actually inherited, as well as validating that fd > STDERR_FILENO. Another missing validation check is for duplicate use. With the monitor command, you ALWAYS have a unique fd (thanks to SCM_RIGHTS). But with the command line, I can type 'qemu -add-fd fd=4,set=1 -add-fd fd=4,set=2'. Oops - I've now corrupted your set layout, unless you validate that every fd requested in -add-fd does not already reside in any existing set. Thinking aloud: On the other hand, being able to pass in one fd to multiple sets MIGHT be useful; in the SCM_RIGHTS monitor command case, I can pass the same fd from the management perspective into multiple sets, even though in qemu's perspective, there will be multiple fds created (one per call). Perhaps instead of directly adding the inherited fd to a set, and having to then sweep all sets to check for duplicates, it might make sense to add dup(fd) to a set, so that if I call: qemu -add-fd fd=4,set=1 -add-fd fd=4,set=2 -add-fd fd=5,set=2 what REALLY happens is that qemu adds dup(4)==6 to set 1, dup(4)==7 to set 2, and dup(5)==8 to set 3. Then, after all ALL -add-fd have been processed, qemu then does another pass through them calling close(4) and close(5) (to avoid holding the original fds open indefinitely if the corresponding sets are discarded). Hmm, notice that if you dup() before adding to a set, then that the dup() resolves my first complaint of validating that the inherited fd exists; it also changes the problem of dealing with fd 0 (it would now be valid to add fd 0 to any number of sets; but a final cleanup loop had better be careful to not call close(0) unintentionally). Personally, though, I'm not sure the complexity of using dup() buys us anything, so I'm happy with the simpler solution of using fd as-is, coupled with a check for no reuse of an fd already in a set.
On 10/10/2012 06:31 PM, Eric Blake wrote: > On 10/10/2012 08:20 AM, 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=4,set=2,opaque="rdwr:/path/to/file" >> -add-fd fd=5,set=2,opaque="rdonly:/path/to/file" >> -drive file=/dev/fdset/2,index=0,media=disk >> >> This example adds 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. > > Missing some argument validation. Earlier, I complained about set > validation, now I'm going to complain about fd validation. > >> +static int parse_add_fd(QemuOpts *opts, void *opaque) >> +{ >> + int fd; >> + 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 I call 'qemu -add-fd fd=-2,set=1', that had better fail because fd -2 > does not exist. Likewise if I call 'qemu -add-fd fd=10000000,set=1' > (here, picking an fd that I know can't be opened). More subtly, 'qemu > -add-fd fd=4,set=1 4<&-' should fail, since fd 4 was not inherited down > to the qemu process. Fuzzier is whether 'qemu -add-fd fd=0,set=1' makes > sense, as that would then make stdin be competing for multiple uses; > this would be a situation that the monitor command can't duplicate, so > you have introduced new ways to possibly break things from the command > line. I'm thinking it's safest to reject fd of 0, 1, or 2 for now, and > only relax it later IF we can prove that it would be both useful and safe. > > So it sounds like you need something like fcntl(fd,F_GETFL) to see that > an the fd was actually inherited, as well as validating that fd > > STDERR_FILENO. I agree. I'll try this approach. > > Another missing validation check is for duplicate use. With the monitor > command, you ALWAYS have a unique fd (thanks to SCM_RIGHTS). But with > the command line, I can type 'qemu -add-fd fd=4,set=1 -add-fd > fd=4,set=2'. Oops - I've now corrupted your set layout, unless you > validate that every fd requested in -add-fd does not already reside in > any existing set. > I don't see this validation check for duplicate use of fd's being necessary. Like you say below, in the QMP add-fd case we can add the same fd multiple times. So we should be able to add the same fd multiple times via the command line. The only difference between QMP and command line in this case is that the QMP fd is a dup and therefore a different number and the command line fd will be the same fd. I'd prefer to leave this alone unless there's a compelling reason to block adding of the same fd. > > > Thinking aloud: > On the other hand, being able to pass in one fd to multiple sets MIGHT > be useful; in the SCM_RIGHTS monitor command case, I can pass the same > fd from the management perspective into multiple sets, even though in > qemu's perspective, there will be multiple fds created (one per call). > Perhaps instead of directly adding the inherited fd to a set, and having > to then sweep all sets to check for duplicates, it might make sense to > add dup(fd) to a set, so that if I call: > > qemu -add-fd fd=4,set=1 -add-fd fd=4,set=2 -add-fd fd=5,set=2 > > what REALLY happens is that qemu adds dup(4)==6 to set 1, dup(4)==7 to > set 2, and dup(5)==8 to set 3. Then, after all ALL -add-fd have been > processed, qemu then does another pass through them calling close(4) and > close(5) (to avoid holding the original fds open indefinitely if the > corresponding sets are discarded). Hmm, notice that if you dup() before > adding to a set, then that the dup() resolves my first complaint of > validating that the inherited fd exists; it also changes the problem of > dealing with fd 0 (it would now be valid to add fd 0 to any number of > sets; but a final cleanup loop had better be careful to not call > close(0) unintentionally). > > Personally, though, I'm not sure the complexity of using dup() buys us > anything, so I'm happy with the simpler solution of using fd as-is, > coupled with a check for no reuse of an fd already in a set. >
On 10/11/2012 08:45 AM, Corey Bryant wrote: >> Another missing validation check is for duplicate use. With the monitor >> command, you ALWAYS have a unique fd (thanks to SCM_RIGHTS). But with >> the command line, I can type 'qemu -add-fd fd=4,set=1 -add-fd >> fd=4,set=2'. Oops - I've now corrupted your set layout, unless you >> validate that every fd requested in -add-fd does not already reside in >> any existing set. >> > > I don't see this validation check for duplicate use of fd's being > necessary. Like you say below, in the QMP add-fd case we can add the > same fd multiple times. So we should be able to add the same fd > multiple times via the command line. The only difference between QMP > and command line in this case is that the QMP fd is a dup and therefore > a different number and the command line fd will be the same fd. I'd > prefer to leave this alone unless there's a compelling reason to block > adding of the same fd. There is a compelling reason to prevent duplicates among your sets: qemu_close(). Suppose I add fd 4 into set 1 and 2, and then discard set 2 via monitor commands. Then, when qemu_close() drops the last reference to set 2, it steps through and calls close() on all fds in that set, including fd 4. Oops - now set 1 is invalid, because it is tracking a closed fd. And worse, if qemu then does something else to open a new fd, it will get fd 4 again, and now set 1 will be tracking the WRONG fd.
On 10/10/2012 04:31 PM, Eric Blake wrote: > Another missing validation check is for duplicate use. With the monitor > command, you ALWAYS have a unique fd (thanks to SCM_RIGHTS). But with > the command line, I can type 'qemu -add-fd fd=4,set=1 -add-fd > fd=4,set=2'. Oops - I've now corrupted your set layout, unless you > validate that every fd requested in -add-fd does not already reside in > any existing set. > > Thinking aloud: > On the other hand, being able to pass in one fd to multiple sets MIGHT > be useful; in the SCM_RIGHTS monitor command case, I can pass the same > fd from the management perspective into multiple sets, even though in > qemu's perspective, there will be multiple fds created (one per call). > Perhaps instead of directly adding the inherited fd to a set, and having > to then sweep all sets to check for duplicates, it might make sense to > add dup(fd) to a set, so that if I call: > > qemu -add-fd fd=4,set=1 -add-fd fd=4,set=2 -add-fd fd=5,set=2 > > what REALLY happens is that qemu adds dup(4)==6 to set 1, dup(4)==7 to > set 2, and dup(5)==8 to set 3. Then, after all ALL -add-fd have been > processed, qemu then does another pass through them calling close(4) and > close(5) (to avoid holding the original fds open indefinitely if the > corresponding sets are discarded). Another idea: a hybrid approach - the _first_ -add-fd 4 directly adds 4 to the set, all other -add-fd 4 end up adding dup(4) instead (well, fcntl(F_DUPFD_CLOEXEC), but you get the picture). That is, do the duplicate scanning, and if there is no duplicate, use the fd directly; if there IS a duplicate, then put a unique fd number as a copy into the remaining sets. That way, you don't have to do a final close() sweep across the -add-fd arguments passed on the command line, and you still don't have to worry about duplicated fds across multiple sets causing mayhem in qemu_close().
On 10/11/2012 10:04 AM, Eric Blake wrote: > Another idea: a hybrid approach - the _first_ -add-fd 4 directly adds 4 > to the set, all other -add-fd 4 end up adding dup(4) instead (well, > fcntl(F_DUPFD_CLOEXEC), but you get the picture). That is, do the > duplicate scanning, and if there is no duplicate, use the fd directly; > if there IS a duplicate, then put a unique fd number as a copy into the > remaining sets. That way, you don't have to do a final close() sweep > across the -add-fd arguments passed on the command line, and you still > don't have to worry about duplicated fds across multiple sets causing > mayhem in qemu_close(). Hmm, you may also need to be careful of corner cases. If I do: qemu -add-fd fd=4,set=1 -add-fd fd=4,set=2 -add-fd fd=5,set=3 5<&- with fd 5 not inherited, then a dup(4) would give 5; you don't want to accidentally add a copy of fd 4 into set 3, but rather fail because fd 5 was not inherited.
On 10/11/2012 11:55 AM, Eric Blake wrote: > On 10/11/2012 08:45 AM, Corey Bryant wrote: > >>> Another missing validation check is for duplicate use. With the monitor >>> command, you ALWAYS have a unique fd (thanks to SCM_RIGHTS). But with >>> the command line, I can type 'qemu -add-fd fd=4,set=1 -add-fd >>> fd=4,set=2'. Oops - I've now corrupted your set layout, unless you >>> validate that every fd requested in -add-fd does not already reside in >>> any existing set. >>> >> >> I don't see this validation check for duplicate use of fd's being >> necessary. Like you say below, in the QMP add-fd case we can add the >> same fd multiple times. So we should be able to add the same fd >> multiple times via the command line. The only difference between QMP >> and command line in this case is that the QMP fd is a dup and therefore >> a different number and the command line fd will be the same fd. I'd >> prefer to leave this alone unless there's a compelling reason to block >> adding of the same fd. > > There is a compelling reason to prevent duplicates among your sets: > qemu_close(). > > Suppose I add fd 4 into set 1 and 2, and then discard set 2 via monitor > commands. Then, when qemu_close() drops the last reference to set 2, it > steps through and calls close() on all fds in that set, including fd 4. > Oops - now set 1 is invalid, because it is tracking a closed fd. And > worse, if qemu then does something else to open a new fd, it will get fd > 4 again, and now set 1 will be tracking the WRONG fd. > Ah yes, that is compelling. So we do need something here. I'll reply to your other email regarding the approach to take.
On 10/11/2012 12:04 PM, Eric Blake wrote: > On 10/10/2012 04:31 PM, Eric Blake wrote: > >> Another missing validation check is for duplicate use. With the monitor >> command, you ALWAYS have a unique fd (thanks to SCM_RIGHTS). But with >> the command line, I can type 'qemu -add-fd fd=4,set=1 -add-fd >> fd=4,set=2'. Oops - I've now corrupted your set layout, unless you >> validate that every fd requested in -add-fd does not already reside in >> any existing set. >> >> Thinking aloud: >> On the other hand, being able to pass in one fd to multiple sets MIGHT >> be useful; in the SCM_RIGHTS monitor command case, I can pass the same >> fd from the management perspective into multiple sets, even though in >> qemu's perspective, there will be multiple fds created (one per call). >> Perhaps instead of directly adding the inherited fd to a set, and having >> to then sweep all sets to check for duplicates, it might make sense to >> add dup(fd) to a set, so that if I call: >> >> qemu -add-fd fd=4,set=1 -add-fd fd=4,set=2 -add-fd fd=5,set=2 >> >> what REALLY happens is that qemu adds dup(4)==6 to set 1, dup(4)==7 to >> set 2, and dup(5)==8 to set 3. Then, after all ALL -add-fd have been >> processed, qemu then does another pass through them calling close(4) and >> close(5) (to avoid holding the original fds open indefinitely if the >> corresponding sets are discarded). > > Another idea: a hybrid approach - the _first_ -add-fd 4 directly adds 4 > to the set, all other -add-fd 4 end up adding dup(4) instead (well, > fcntl(F_DUPFD_CLOEXEC), but you get the picture). That is, do the > duplicate scanning, and if there is no duplicate, use the fd directly; > if there IS a duplicate, then put a unique fd number as a copy into the > remaining sets. That way, you don't have to do a final close() sweep > across the -add-fd arguments passed on the command line, and you still > don't have to worry about duplicated fds across multiple sets causing > mayhem in qemu_close(). > This would simplify the code, but I wonder if it would be confusing to users when they call query-fdsets and only see a single fd 4. It may make more sense to dup all fds that are passed with -add-fd, and then it basically works the same as the QMP add-fd via SCM_RIGHTS. On a somewhat related note, one major difference between the QMP add-fd and command line -add-fd, is that -add-fd doesn't return the fd that was added. So opaque strings will be even more important when passing fds on the command line.
diff --git a/qemu-config.c b/qemu-config.c index cd1ec21..0bd67ca 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 to add 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..884fcb6 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=4,set=2,opaque="rdwr:/path/to/file" +-add-fd fd=5,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,33 @@ 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 that is to be added to the fd set. +@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=4,set=2,opaque="rdwr:/path/to/file" +-add-fd fd=5,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 8d305ca..0265712 100644 --- a/vl.c +++ b/vl.c @@ -790,6 +790,33 @@ static int parse_sandbox(QemuOpts *opts, void *opaque) return 0; } +static int parse_add_fd(QemuOpts *opts, void *opaque) +{ + int fd; + 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 == -1) { + qerror_report(ERROR_CLASS_GENERIC_ERROR, "fd option is required"); + return -1; + } + + if (fdset_id == -1) { + qerror_report(ERROR_CLASS_GENERIC_ERROR, "set option is required"); + return -1; + } + + /* add the fd, and optionally the opaque string, to the fd set */ + monitor_fdset_add_fd(fd, true, fdset_id, fd_opaque ? true : false, + fd_opaque); + + return 0; +} + /***********************************************************/ /* QEMU Block devices */ @@ -3299,6 +3326,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); } @@ -3310,6 +3342,10 @@ 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 (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=4,set=2,opaque="rdwr:/path/to/file" -add-fd fd=5,set=2,opaque="rdonly:/path/to/file" -drive file=/dev/fdset/2,index=0,media=disk This example adds 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) qemu-config.c | 22 ++++++++++++++++++++++ qemu-options.hx | 35 +++++++++++++++++++++++++++++++++++ vl.c | 36 ++++++++++++++++++++++++++++++++++++ 3 files changed, 93 insertions(+)