[Qemu-devel] [PATCH 0/4] command line fd passing using fd sets

[Qemu-devel] [PATCH 0/4] command line fd passing using fd sets

[Corey Bryant]

This series adds command line file descriptor passing support
to the -drive 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

An example of using the new -drive fd and opaque options:
qemu-kvm -drive fd=24,opaque="rdwr:/path/file",index=0,media=disk

When an fd, and optionally opaque, are passed with -drive, they
are added to an automatically generated fd set.  The file name
is also generated in the form of "/dev/fdset/nnn", where nnn is
the fd set ID.  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.

Corey Bryant (4):
  monitor: Less restrictive fd matching for fd sets
  monitor: Enable adding an inherited fd to an fd set
  qemu-config: Add -drive fd and opaque options
  blockdev: Process -drive fd and opaque options

 blockdev.c      |  40 +++++++++++++++++-
 monitor.c       | 128 ++++++++++++++++++++++++++++++++------------------------
 monitor.h       |   3 ++
 qemu-config.c   |   8 ++++
 qemu-options.hx |  15 ++++++-
 5 files changed, 135 insertions(+), 59 deletions(-)

-- 
1.7.11.4

[Qemu-devel] [PATCH 1/4] monitor: Less restrictive fd matching for fd sets

[Corey Bryant]

Currently, in order to use a file descriptor that resides in an
fd set, the access mode flag of the qemu_open() call has to be
an exact match of the access mode flag of an fd in the requested
fd set.

This patch lightens up that restriction.  For example, if QEMU
attempts to qemu_open() "/dev/fdset/2" with O_RDONLY or O_WRONLY,
and an fd in fd set 2 has O_RDWR, the call will now be successful.

Signed-off-by: Corey Bryant <coreyb@linux.vnet.ibm.com>
---
 monitor.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/monitor.c b/monitor.c
index a0e3ffb..34a968c 100644
--- a/monitor.c
+++ b/monitor.c
@@ -2304,7 +2304,8 @@ int monitor_fdset_get_fd(int64_t fdset_id, int flags)
                 return -1;
             }
 
-            if ((flags & O_ACCMODE) == (mon_fd_flags & O_ACCMODE)) {
+            if ((mon_fd_flags & O_ACCMODE) == O_RDWR ||
+                (mon_fd_flags & O_ACCMODE) == (flags & O_ACCMODE)) {
                 return mon_fdset_fd->fd;
             }
         }
-- 
1.7.11.4

Re: [Qemu-devel] [PATCH 1/4] monitor: Less restrictive fd matching for fd sets

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1932 bytes --]

On 10/05/2012 12:07 PM, Corey Bryant wrote:
> Currently, in order to use a file descriptor that resides in an
> fd set, the access mode flag of the qemu_open() call has to be
> an exact match of the access mode flag of an fd in the requested
> fd set.
> 
> This patch lightens up that restriction.  For example, if QEMU
> attempts to qemu_open() "/dev/fdset/2" with O_RDONLY or O_WRONLY,
> and an fd in fd set 2 has O_RDWR, the call will now be successful.

I was right for wondering if we'd need this, back when fdsets were first
implemented. :)

However...

> 
> Signed-off-by: Corey Bryant <coreyb@linux.vnet.ibm.com>
> ---
>  monitor.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/monitor.c b/monitor.c
> index a0e3ffb..34a968c 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -2304,7 +2304,8 @@ int monitor_fdset_get_fd(int64_t fdset_id, int flags)
>                  return -1;
>              }
>  
> -            if ((flags & O_ACCMODE) == (mon_fd_flags & O_ACCMODE)) {
> +            if ((mon_fd_flags & O_ACCMODE) == O_RDWR ||
> +                (mon_fd_flags & O_ACCMODE) == (flags & O_ACCMODE)) {
>                  return mon_fdset_fd->fd;
>              }

Is this always the right fd to return?  Suppose I create a set that
contains two fds, one O_RDONLY and another O_RDWR; and that the current
qemu_open() is requesting O_RDONLY.  It feels like we would want to get
the O_RDONLY fd returned, even if it comes later in the set, since it is
nominally safer.

That is, I wonder if this should really be implemented as an iteration
over the set that selects the best possible match, where more than one
fd can match, but at the end of the iteration, we have the closest match
possible, rather than the first (possibly inexact) match.

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 617 bytes --]

Re: [Qemu-devel] [PATCH 1/4] monitor: Less restrictive fd matching for fd sets

[Corey Bryant]

On 10/05/2012 02:35 PM, Eric Blake wrote:
> On 10/05/2012 12:07 PM, Corey Bryant wrote:
>> Currently, in order to use a file descriptor that resides in an
>> fd set, the access mode flag of the qemu_open() call has to be
>> an exact match of the access mode flag of an fd in the requested
>> fd set.
>>
>> This patch lightens up that restriction.  For example, if QEMU
>> attempts to qemu_open() "/dev/fdset/2" with O_RDONLY or O_WRONLY,
>> and an fd in fd set 2 has O_RDWR, the call will now be successful.
>
> I was right for wondering if we'd need this, back when fdsets were first
> implemented. :)

Yeah yeah yeah :)

>
> However...
>
>>
>> Signed-off-by: Corey Bryant <coreyb@linux.vnet.ibm.com>
>> ---
>>   monitor.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/monitor.c b/monitor.c
>> index a0e3ffb..34a968c 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -2304,7 +2304,8 @@ int monitor_fdset_get_fd(int64_t fdset_id, int flags)
>>                   return -1;
>>               }
>>
>> -            if ((flags & O_ACCMODE) == (mon_fd_flags & O_ACCMODE)) {
>> +            if ((mon_fd_flags & O_ACCMODE) == O_RDWR ||
>> +                (mon_fd_flags & O_ACCMODE) == (flags & O_ACCMODE)) {
>>                   return mon_fdset_fd->fd;
>>               }
>
> Is this always the right fd to return?  Suppose I create a set that
> contains two fds, one O_RDONLY and another O_RDWR; and that the current
> qemu_open() is requesting O_RDONLY.  It feels like we would want to get
> the O_RDONLY fd returned, even if it comes later in the set, since it is
> nominally safer.
>
> That is, I wonder if this should really be implemented as an iteration
> over the set that selects the best possible match, where more than one
> fd can match, but at the end of the iteration, we have the closest match
> possible, rather than the first (possibly inexact) match.
>

Actually I think we can go back to requiring an exact match if we use 
your new proposal of adding a new command that allows multiple fd's to 
be added to an fd set over the command line.

-- 
Regards,
Corey Bryant

[Qemu-devel] [PATCH 2/4] monitor: Enable adding an inherited fd to an fd set

[Corey Bryant]

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.

This patch also prevents removal of an fd from an fd set during
initialization.  This allows the fd to remain in the fd set after
probing of the image file.

Signed-off-by: Corey Bryant <coreyb@linux.vnet.ibm.com>
---
 monitor.c | 125 +++++++++++++++++++++++++++++++++++---------------------------
 monitor.h |   3 ++
 2 files changed, 74 insertions(+), 54 deletions(-)

diff --git a/monitor.c b/monitor.c
index 34a968c..870fc15 100644
--- a/monitor.c
+++ b/monitor.c
@@ -2111,8 +2111,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);
@@ -2141,8 +2142,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;
-    MonFdsetFd *mon_fdset_fd;
     AddfdInfo *fdinfo;
 
     fd = qemu_chr_fe_get_msgfd(mon->chr);
@@ -2151,57 +2150,11 @@ 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) {
-            if (mon_fdset->id == fdset_id) {
-                break;
-            }
-        }
-        if (mon_fdset == NULL) {
-            error_set(errp, QERR_INVALID_PARAMETER_VALUE, "fdset-id",
-                      "an existing fdset-id");
-            goto error;
-        }
-    } else {
-        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;
-            }
-            break;
-        }
-
-        mon_fdset = g_malloc0(sizeof(*mon_fdset));
-        mon_fdset->id = fdset_id_prev + 1;
-
-        /* The fdset list is ordered by fdset ID */
-        if (mon_fdset->id == 0) {
-            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);
+    fdinfo = monitor_fdset_add_fd(fd, has_fdset_id, fdset_id, has_opaque,
+                                  opaque, errp);
+    if (fdinfo) {
+        return fdinfo;
     }
-    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) {
@@ -2287,6 +2240,70 @@ 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;
+    MonFdsetFd *mon_fdset_fd;
+    AddfdInfo *fdinfo;
+
+    if (has_fdset_id) {
+        QLIST_FOREACH(mon_fdset, &mon_fdsets, next) {
+            if (mon_fdset->id == fdset_id) {
+                break;
+            }
+        }
+        if (mon_fdset == NULL) {
+            error_set(errp, QERR_INVALID_PARAMETER_VALUE, "fdset-id",
+                      "an existing fdset-id");
+            goto error;
+        }
+    } else {
+        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;
+            }
+            break;
+        }
+
+        mon_fdset = g_malloc0(sizeof(*mon_fdset));
+        mon_fdset->id = fdset_id_prev + 1;
+
+        /* The fdset list is ordered by fdset ID */
+        if (mon_fdset->id == 0) {
+            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:
+    return NULL;
+}
+
 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

[Qemu-devel] [PATCH 3/4] qemu-config: Add -drive fd and opaque options

[Corey Bryant]

These new options can be used for passing drive file descriptors
on the command line, instead of using the file option to specify
a file name.

These new command line options mirror the existing add-fd QMP
command which allows an fd to be passed to QEMU via SCM_RIGHTS and
added to an fd set.  The opaque option is also available with
add-fd, and allows a free-form string to be stored in the fd set
along with the fd.

Signed-off-by: Corey Bryant <coreyb@linux.vnet.ibm.com>
---
 qemu-config.c   |  8 ++++++++
 qemu-options.hx | 15 +++++++++++++--
 2 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/qemu-config.c b/qemu-config.c
index cd1ec21..91053dd 100644
--- a/qemu-config.c
+++ b/qemu-config.c
@@ -114,6 +114,14 @@ static QemuOptsList qemu_drive_opts = {
             .name = "copy-on-read",
             .type = QEMU_OPT_BOOL,
             .help = "copy read data from backing file into image file",
+        },{
+            .name = "fd",
+            .type = QEMU_OPT_NUMBER,
+            .help = "disk image file descriptor",
+        },{
+            .name = "opaque",
+            .type = QEMU_OPT_STRING,
+            .help = "free-form string used to describe fd",
         },
         { /* end of list */ }
     },
diff --git a/qemu-options.hx b/qemu-options.hx
index 7d97f96..513530f 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -149,7 +149,7 @@ using @file{/dev/cdrom} as filename (@pxref{host_drives}).
 ETEXI
 
 DEF("drive", HAS_ARG, QEMU_OPTION_drive,
-    "-drive [file=file][,if=type][,bus=n][,unit=m][,media=d][,index=i]\n"
+    "-drive [file=file|fd=fd[,opaque=o]][,if=type][,bus=n][,unit=m][,media=d][,index=i]\n"
     "       [,cyls=c,heads=h,secs=s[,trans=t]][,snapshot=on|off]\n"
     "       [,cache=writethrough|writeback|none|directsync|unsafe][,format=f]\n"
     "       [,serial=s][,addr=A][,id=name][,aio=threads|native]\n"
@@ -170,6 +170,12 @@ this drive. If the filename contains comma, you must double it
 
 Special files such as iSCSI devices can be specified using protocol
 specific URLs. See the section for "Device URL Syntax" for more information.
+@item fd=@var{fd}
+This option defines which disk image (@pxref{disk_images}) file descriptor to
+use with this drive.
+@item opaque=@var{opaque}
+This option defines a free-form string that describes @var{fd}.  This is used
+when storing @var{fd} in a file descriptor set.
 @item if=@var{interface}
 This option defines on which type on interface the drive is connected.
 Available types are: ide, scsi, sd, mtd, floppy, pflash, virtio.
@@ -257,12 +263,17 @@ 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 a pre-opened file descriptor:
+@example
+qemu-system-i386 -drive fd=24,opaque="rdwr:/path/to/file",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
 @end example
 
-If you don't specify the "file=" argument, you define an empty drive:
+If you don't specify the "file=" or "fd=" arguments, you define an empty drive:
 @example
 qemu-system-i386 -drive if=ide,index=1,media=cdrom
 @end example
-- 
1.7.11.4

Re: [Qemu-devel] [PATCH 3/4] qemu-config: Add -drive fd and opaque options

[Blue Swirl]

On Fri, Oct 5, 2012 at 6:07 PM, Corey Bryant <coreyb@linux.vnet.ibm.com> wrote:
> These new options can be used for passing drive file descriptors
> on the command line, instead of using the file option to specify
> a file name.
>
> These new command line options mirror the existing add-fd QMP
> command which allows an fd to be passed to QEMU via SCM_RIGHTS and
> added to an fd set.  The opaque option is also available with
> add-fd, and allows a free-form string to be stored in the fd set
> along with the fd.
>
> Signed-off-by: Corey Bryant <coreyb@linux.vnet.ibm.com>
> ---
>  qemu-config.c   |  8 ++++++++
>  qemu-options.hx | 15 +++++++++++++--
>  2 files changed, 21 insertions(+), 2 deletions(-)
>
> diff --git a/qemu-config.c b/qemu-config.c
> index cd1ec21..91053dd 100644
> --- a/qemu-config.c
> +++ b/qemu-config.c
> @@ -114,6 +114,14 @@ static QemuOptsList qemu_drive_opts = {
>              .name = "copy-on-read",
>              .type = QEMU_OPT_BOOL,
>              .help = "copy read data from backing file into image file",
> +        },{
> +            .name = "fd",
> +            .type = QEMU_OPT_NUMBER,
> +            .help = "disk image file descriptor",
> +        },{
> +            .name = "opaque",

'opaque' is not very descriptive and it's also not obvious (except
from the help text) that it's only interesting for file descriptors.
How about fd_name, fd_tag or fd_descr?

> +            .type = QEMU_OPT_STRING,
> +            .help = "free-form string used to describe fd",
>          },
>          { /* end of list */ }
>      },
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 7d97f96..513530f 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -149,7 +149,7 @@ using @file{/dev/cdrom} as filename (@pxref{host_drives}).
>  ETEXI
>
>  DEF("drive", HAS_ARG, QEMU_OPTION_drive,
> -    "-drive [file=file][,if=type][,bus=n][,unit=m][,media=d][,index=i]\n"
> +    "-drive [file=file|fd=fd[,opaque=o]][,if=type][,bus=n][,unit=m][,media=d][,index=i]\n"
>      "       [,cyls=c,heads=h,secs=s[,trans=t]][,snapshot=on|off]\n"
>      "       [,cache=writethrough|writeback|none|directsync|unsafe][,format=f]\n"
>      "       [,serial=s][,addr=A][,id=name][,aio=threads|native]\n"
> @@ -170,6 +170,12 @@ this drive. If the filename contains comma, you must double it
>
>  Special files such as iSCSI devices can be specified using protocol
>  specific URLs. See the section for "Device URL Syntax" for more information.
> +@item fd=@var{fd}
> +This option defines which disk image (@pxref{disk_images}) file descriptor to
> +use with this drive.
> +@item opaque=@var{opaque}
> +This option defines a free-form string that describes @var{fd}.  This is used
> +when storing @var{fd} in a file descriptor set.
>  @item if=@var{interface}
>  This option defines on which type on interface the drive is connected.
>  Available types are: ide, scsi, sd, mtd, floppy, pflash, virtio.
> @@ -257,12 +263,17 @@ 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 a pre-opened file descriptor:
> +@example
> +qemu-system-i386 -drive fd=24,opaque="rdwr:/path/to/file",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
>  @end example
>
> -If you don't specify the "file=" argument, you define an empty drive:
> +If you don't specify the "file=" or "fd=" arguments, you define an empty drive:
>  @example
>  qemu-system-i386 -drive if=ide,index=1,media=cdrom
>  @end example
> --
> 1.7.11.4
>
>

Re: [Qemu-devel] [PATCH 3/4] qemu-config: Add -drive fd and opaque options

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1385 bytes --]

On 10/05/2012 12:25 PM, Blue Swirl wrote:
> On Fri, Oct 5, 2012 at 6:07 PM, Corey Bryant <coreyb@linux.vnet.ibm.com> wrote:
>> These new options can be used for passing drive file descriptors
>> on the command line, instead of using the file option to specify
>> a file name.
>>
>> These new command line options mirror the existing add-fd QMP
>> command which allows an fd to be passed to QEMU via SCM_RIGHTS and
>> added to an fd set.  The opaque option is also available with
>> add-fd, and allows a free-form string to be stored in the fd set
>> along with the fd.
>>

>> +            .name = "opaque",
> 
> 'opaque' is not very descriptive and it's also not obvious (except
> from the help text) that it's only interesting for file descriptors.
> How about fd_name, fd_tag or fd_descr?

Hmm, since opaque is per-fd in the existing monitor command, that means
my proposal needs a slight modification to:

 -fdset set=1,fd=24,opaque="rdonly",fd=25,opaque="rdwr"

or some other way where we can specify multiple fds and multiple opaque
strings per set.

At any rate, this just proves that we need to nail down the command line
implementation to something that is easy enough to use, before coding up
something that locks us in to bad design.

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 617 bytes --]

Re: [Qemu-devel] [PATCH 3/4] qemu-config: Add -drive fd and opaque options

[Corey Bryant]

On 10/05/2012 02:30 PM, Eric Blake wrote:
> On 10/05/2012 12:25 PM, Blue Swirl wrote:
>> On Fri, Oct 5, 2012 at 6:07 PM, Corey Bryant <coreyb@linux.vnet.ibm.com> wrote:
>>> These new options can be used for passing drive file descriptors
>>> on the command line, instead of using the file option to specify
>>> a file name.
>>>
>>> These new command line options mirror the existing add-fd QMP
>>> command which allows an fd to be passed to QEMU via SCM_RIGHTS and
>>> added to an fd set.  The opaque option is also available with
>>> add-fd, and allows a free-form string to be stored in the fd set
>>> along with the fd.
>>>
>
>>> +            .name = "opaque",
>>
>> 'opaque' is not very descriptive and it's also not obvious (except
>> from the help text) that it's only interesting for file descriptors.
>> How about fd_name, fd_tag or fd_descr?
>
> Hmm, since opaque is per-fd in the existing monitor command, that means
> my proposal needs a slight modification to:
>
>   -fdset set=1,fd=24,opaque="rdonly",fd=25,opaque="rdwr"

Yes, this makes more sense.  I'd like to mirror the add-fd QMP command 
as much as possible:

{ 'command': 'add-fd', 'data': {'*fdset-id': 'int', '*opaque': 'str'},
   'returns': 'AddfdInfo' }

So maybe we can make it:

-add-fd fd=24,fdset-id=1,opaque="rdonly" -add-fd 
fd=25,fdset-id=1,opaque="rdwr"

-- 
Regards,
Corey Bryant

>
> or some other way where we can specify multiple fds and multiple opaque
> strings per set.
>
> At any rate, this just proves that we need to nail down the command line
> implementation to something that is easy enough to use, before coding up
> something that locks us in to bad design.
>

Re: [Qemu-devel] [PATCH 3/4] qemu-config: Add -drive fd and opaque options

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1212 bytes --]

On 10/05/2012 12:44 PM, Corey Bryant wrote:
> Yes, this makes more sense.  I'd like to mirror the add-fd QMP command
> as much as possible:
> 
> { 'command': 'add-fd', 'data': {'*fdset-id': 'int', '*opaque': 'str'},
>   'returns': 'AddfdInfo' }
> 
> So maybe we can make it:
> 
> -add-fd fd=24,fdset-id=1,opaque="rdonly" -add-fd
> fd=25,fdset-id=1,opaque="rdwr"

Sounds better.  Note that while fdset-id is optional in QMP, I think it
needs to be mandatory on the CLI (you're telling qemu to use an
inherited fd, but unless you know what set that fd belongs to, you can't
then refer to that fd elsewhere on the command line, and unlike the QMP
command, there is no venue for QMP to tell you what set was autocreated).

Bike-shedding: I think the command line can be slightly shorter with:

-add-fd fd=24,set=1,opaque=...

with no real loss in information (that is, s/fdset-id/set/), since our
command lines are already quite long.  But going longhand to match QMP
doesn't hurt my feelings (libvirt will be automating this all anyway, so
I won't really be typing it by hand).

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 617 bytes --]

Re: [Qemu-devel] [PATCH 3/4] qemu-config: Add -drive fd and opaque options

[Corey Bryant]

On 10/05/2012 02:51 PM, Eric Blake wrote:
> On 10/05/2012 12:44 PM, Corey Bryant wrote:
>> Yes, this makes more sense.  I'd like to mirror the add-fd QMP command
>> as much as possible:
>>
>> { 'command': 'add-fd', 'data': {'*fdset-id': 'int', '*opaque': 'str'},
>>    'returns': 'AddfdInfo' }
>>
>> So maybe we can make it:
>>
>> -add-fd fd=24,fdset-id=1,opaque="rdonly" -add-fd
>> fd=25,fdset-id=1,opaque="rdwr"
>
> Sounds better.  Note that while fdset-id is optional in QMP, I think it
> needs to be mandatory on the CLI (you're telling qemu to use an
> inherited fd, but unless you know what set that fd belongs to, you can't
> then refer to that fd elsewhere on the command line, and unlike the QMP
> command, there is no venue for QMP to tell you what set was autocreated).
>

I agree the fdset ID should be mandatory on the command line so we can 
link up other commands like '-drive file=/dev/fdset/nnn' where nnn is 
the fdset ID.

> Bike-shedding: I think the command line can be slightly shorter with:
>
> -add-fd fd=24,set=1,opaque=...
>

Sure, I have no problem with shortening it.

-- 
Regards,
Corey Bryant

> with no real loss in information (that is, s/fdset-id/set/), since our
> command lines are already quite long.  But going longhand to match QMP
> doesn't hurt my feelings (libvirt will be automating this all anyway, so
> I won't really be typing it by hand).
>

Re: [Qemu-devel] [PATCH 3/4] qemu-config: Add -drive fd and opaque options

[Corey Bryant]

On 10/05/2012 02:25 PM, Blue Swirl wrote:
> On Fri, Oct 5, 2012 at 6:07 PM, Corey Bryant <coreyb@linux.vnet.ibm.com> wrote:
>> These new options can be used for passing drive file descriptors
>> on the command line, instead of using the file option to specify
>> a file name.
>>
>> These new command line options mirror the existing add-fd QMP
>> command which allows an fd to be passed to QEMU via SCM_RIGHTS and
>> added to an fd set.  The opaque option is also available with
>> add-fd, and allows a free-form string to be stored in the fd set
>> along with the fd.
>>
>> Signed-off-by: Corey Bryant <coreyb@linux.vnet.ibm.com>
>> ---
>>   qemu-config.c   |  8 ++++++++
>>   qemu-options.hx | 15 +++++++++++++--
>>   2 files changed, 21 insertions(+), 2 deletions(-)
>>
>> diff --git a/qemu-config.c b/qemu-config.c
>> index cd1ec21..91053dd 100644
>> --- a/qemu-config.c
>> +++ b/qemu-config.c
>> @@ -114,6 +114,14 @@ static QemuOptsList qemu_drive_opts = {
>>               .name = "copy-on-read",
>>               .type = QEMU_OPT_BOOL,
>>               .help = "copy read data from backing file into image file",
>> +        },{
>> +            .name = "fd",
>> +            .type = QEMU_OPT_NUMBER,
>> +            .help = "disk image file descriptor",
>> +        },{
>> +            .name = "opaque",
>
> 'opaque' is not very descriptive and it's also not obvious (except
> from the help text) that it's only interesting for file descriptors.
> How about fd_name, fd_tag or fd_descr?
>

I'd like to mirror the add-fd QMP command as much as possible:

{ 'command': 'add-fd', 'data': {'*fdset-id': 'int', '*opaque': 'str'},
   'returns': 'AddfdInfo' }

And I think use of opaque will make more sense if I don't merge these 
options in to -drive, and instead create a new command specifically for 
adding fds, like Eric is suggesting.

-- 
Regards,
Corey Bryant

>> +            .type = QEMU_OPT_STRING,
>> +            .help = "free-form string used to describe fd",
>>           },
>>           { /* end of list */ }
>>       },
>> diff --git a/qemu-options.hx b/qemu-options.hx
>> index 7d97f96..513530f 100644
>> --- a/qemu-options.hx
>> +++ b/qemu-options.hx
>> @@ -149,7 +149,7 @@ using @file{/dev/cdrom} as filename (@pxref{host_drives}).
>>   ETEXI
>>
>>   DEF("drive", HAS_ARG, QEMU_OPTION_drive,
>> -    "-drive [file=file][,if=type][,bus=n][,unit=m][,media=d][,index=i]\n"
>> +    "-drive [file=file|fd=fd[,opaque=o]][,if=type][,bus=n][,unit=m][,media=d][,index=i]\n"
>>       "       [,cyls=c,heads=h,secs=s[,trans=t]][,snapshot=on|off]\n"
>>       "       [,cache=writethrough|writeback|none|directsync|unsafe][,format=f]\n"
>>       "       [,serial=s][,addr=A][,id=name][,aio=threads|native]\n"
>> @@ -170,6 +170,12 @@ this drive. If the filename contains comma, you must double it
>>
>>   Special files such as iSCSI devices can be specified using protocol
>>   specific URLs. See the section for "Device URL Syntax" for more information.
>> +@item fd=@var{fd}
>> +This option defines which disk image (@pxref{disk_images}) file descriptor to
>> +use with this drive.
>> +@item opaque=@var{opaque}
>> +This option defines a free-form string that describes @var{fd}.  This is used
>> +when storing @var{fd} in a file descriptor set.
>>   @item if=@var{interface}
>>   This option defines on which type on interface the drive is connected.
>>   Available types are: ide, scsi, sd, mtd, floppy, pflash, virtio.
>> @@ -257,12 +263,17 @@ 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 a pre-opened file descriptor:
>> +@example
>> +qemu-system-i386 -drive fd=24,opaque="rdwr:/path/to/file",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
>>   @end example
>>
>> -If you don't specify the "file=" argument, you define an empty drive:
>> +If you don't specify the "file=" or "fd=" arguments, you define an empty drive:
>>   @example
>>   qemu-system-i386 -drive if=ide,index=1,media=cdrom
>>   @end example
>> --
>> 1.7.11.4
>>
>>
>

[Qemu-devel] [PATCH 4/4] blockdev: Process -drive fd and opaque options

[Corey Bryant]

When an fd, and optionally opaque, are passed with -drive, they
are added to an automatically generated fd set.  The file name
is also generated in the form of "/dev/fdset/nnn", where nnn is
the fd set ID.  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>
---
 blockdev.c | 40 ++++++++++++++++++++++++++++++++++++++--
 1 file changed, 38 insertions(+), 2 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 5f18dfa..cf348c6 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -281,6 +281,7 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
     const char *file = NULL;
     const char *serial;
     const char *mediastr = "";
+    const char *opaque = NULL;
     BlockInterfaceType type;
     enum { MEDIA_DISK, MEDIA_CDROM } media;
     int bus_id, unit_id;
@@ -297,6 +298,10 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
     int snapshot = 0;
     bool copy_on_read;
     int ret;
+    int fd;
+    char filename[32];
+    AddfdInfo *fdinfo = NULL;
+    Error *errp = NULL;
 
     translation = BIOS_ATA_TRANSLATION_AUTO;
     media = MEDIA_DISK;
@@ -315,6 +320,8 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
     copy_on_read = qemu_opt_get_bool(opts, "copy-on-read", false);
 
     file = qemu_opt_get(opts, "file");
+    fd = qemu_opt_get_number(opts, "fd", -1);
+    opaque = qemu_opt_get(opts, "opaque");
     serial = qemu_opt_get(opts, "serial");
 
     if ((buf = qemu_opt_get(opts, "if")) != NULL) {
@@ -575,9 +582,35 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
     default:
         abort();
     }
-    if (!file || !*file) {
-        return dinfo;
+
+    if (file && fd != -1) {
+        error_report("file cannot be used with fd");
+        goto err;
+    }
+
+    if (opaque && fd == -1) {
+        error_report("opaque cannot be specified without fd");
+        goto err;
+    }
+
+    if ((!file || !*file)) {
+        if (fd != -1) {
+            /* add the fd, and optionally opaque, to an fd set */
+            fdinfo = monitor_fdset_add_fd(fd, false, -1, opaque ? true : false,
+                                          opaque, &errp);
+            if (fdinfo == NULL) {
+                goto err;
+            }
+
+            /* set file name to /dev/fdset/nnn */
+            snprintf(filename, sizeof(filename), "/dev/fdset/%" PRId64,
+                     fdinfo->fdset_id);
+            file = filename;
+        } else {
+            return dinfo;
+        }
     }
+
     if (snapshot) {
         /* always use cache=unsafe with snapshot */
         bdrv_flags &= ~BDRV_O_CACHE_MASK;
@@ -625,6 +658,9 @@ err:
     g_free(dinfo->id);
     QTAILQ_REMOVE(&drives, dinfo, next);
     g_free(dinfo);
+    if (fdinfo) {
+        qmp_remove_fd(fdinfo->fdset_id, true, fdinfo->fd, &errp);
+    }
     return NULL;
 }
 
-- 
1.7.11.4

Re: [Qemu-devel] [PATCH 0/4] command line fd passing using fd sets

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1260 bytes --]

On 10/05/2012 12:07 PM, Corey Bryant wrote:
> This series adds command line file descriptor passing support
> to the -drive 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
> 
> An example of using the new -drive fd and opaque options:
> qemu-kvm -drive fd=24,opaque="rdwr:/path/file",index=0,media=disk

This feels wrong.  Now you have to special-case encode the
fd=nn,opaque=xyz handling to EVERY command line argument that takes a
file name, not just -drive.

I'd much rather see:

qemu-kvm -fdset set=1,fds=24,25 \
  -drive file=/def/fdset/1,index=0,media=disk

Where the creation of fdsets happens independently from use of those
sets, and therefore all other arguments that take file names can just
magically take the /dev/fdset/nnn notation that we already support from
the monitor.

Besides, my approach will let me pass in an O_RDONLY fd on 24 and O_RDWR
on 25 into the same set, whereas your approach creates a new set per fd,
so I can't add new fds to the set until the monitor is up and running.

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 617 bytes --]

Re: [Qemu-devel] [PATCH 0/4] command line fd passing using fd sets

[Corey Bryant]

On 10/05/2012 02:26 PM, Eric Blake wrote:
> On 10/05/2012 12:07 PM, Corey Bryant wrote:
>> This series adds command line file descriptor passing support
>> to the -drive 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
>>
>> An example of using the new -drive fd and opaque options:
>> qemu-kvm -drive fd=24,opaque="rdwr:/path/file",index=0,media=disk
>
> This feels wrong.  Now you have to special-case encode the
> fd=nn,opaque=xyz handling to EVERY command line argument that takes a
> file name, not just -drive.
>
> I'd much rather see:
>
> qemu-kvm -fdset set=1,fds=24,25 \
>    -drive file=/def/fdset/1,index=0,media=disk
>
> Where the creation of fdsets happens independently from use of those
> sets, and therefore all other arguments that take file names can just
> magically take the /dev/fdset/nnn notation that we already support from
> the monitor.
>
> Besides, my approach will let me pass in an O_RDONLY fd on 24 and O_RDWR
> on 25 into the same set, whereas your approach creates a new set per fd,
> so I can't add new fds to the set until the monitor is up and running.
>

Thanks for the quick feedback.  I like your idea here.  Much more flexible.

-- 
Regards,
Corey Bryant