On 10/05/2012 12:07 PM, Corey Bryant wrote: I was right for wondering if we'd need this, back when fdsets were first implemented. :) However... 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(a)redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

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(a)linux.vnet.ibm.com&gt; --- 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

On Fri, Oct 5, 2012 at 6:07 PM, Corey Bryant <coreyb(a)linux.vnet.ibm.com&gt; wrote: '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?

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(a)linux.vnet.ibm.com&gt; --- 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