Patchwork [v3,4/4] qemu-config: Add new -add-fd command line option

login
register
mail settings
Submitter Corey Bryant
Date Oct. 16, 2012, 6:10 p.m.
Message ID <1350411046-2453-1-git-send-email-coreyb@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/191840/
State New
Headers show

Comments

Corey Bryant - Oct. 16, 2012, 6:10 p.m.
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(+)
Eric Blake - Oct. 17, 2012, 4:16 a.m.
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?
Kevin Wolf - Oct. 17, 2012, 2:02 p.m.
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
Eric Blake - Oct. 17, 2012, 3:01 p.m.
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.
Kevin Wolf - Oct. 17, 2012, 3:03 p.m.
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
Corey Bryant - Oct. 18, 2012, 1:48 p.m.
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.
Corey Bryant - Oct. 18, 2012, 2:29 p.m.
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.
Corey Bryant - Oct. 18, 2012, 2:34 p.m.
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.
Corey Bryant - Oct. 18, 2012, 6:50 p.m.
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

Patch

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);