[libvirt] [PATCH 0/4] command line fd passing using fd sets
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
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
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
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
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
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
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
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
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.
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
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).
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
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
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
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
participants (3)
-
Blue Swirl -
Corey Bryant -
Eric Blake