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

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

[Corey Bryant]

This series adds command line file descriptor passing support
via a new -add-fd 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

The new -add-fd option is designed to mirror the add-fd QMP
option as much as possible.

Corey Bryant (4):
  monitor: Allow add-fd to any specified fd set
  monitor: Enable adding an inherited fd to an fd set
  monitor: Prevent removing fd from set during init
  qemu-config: Add new -add-fd command line option

 monitor.c        | 142 ++++++++++++++++++++++++++++++++++---------------------
 monitor.h        |   3 ++
 qapi-schema.json |   2 +-
 qemu-config.c    |  22 +++++++++
 qemu-options.hx  |  36 ++++++++++++++
 vl.c             |  78 ++++++++++++++++++++++++++++++
 6 files changed, 228 insertions(+), 55 deletions(-)

-- 
1.7.11.4

[Qemu-devel] [PATCH v4 1/4] monitor: Allow add-fd to any specified fd set

[Corey Bryant]

The first call to add an fd to an fd set was previously not
allowed to choose the fd set ID.  The ID was generated as
the first available and ensuing calls could add more fds by
specifying the fd set ID.  This change allows users to
choose the fd set ID on the first call.

Signed-off-by: Corey Bryant <coreyb@linux.vnet.ibm.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
v2:
 -This patch is new in v2

v3:
 -Added fdset-id bounds checking (eblake@redhat.com)
 -Move 'if (!mon_fdset_cur)' change to this patch
  (kwolf@redhat.com)
 -Add break from for loop if fdset_id is less than mon_fdset->id
  (eblake@redhat.com)

v4:
 -No changes

 monitor.c        | 54 +++++++++++++++++++++++++++++++++++++-----------------
 qapi-schema.json |  2 +-
 2 files changed, 38 insertions(+), 18 deletions(-)

diff --git a/monitor.c b/monitor.c
index 131b325..2e3248f 100644
--- a/monitor.c
+++ b/monitor.c
@@ -2135,7 +2135,7 @@ AddfdInfo *qmp_add_fd(bool has_fdset_id, int64_t fdset_id, bool has_opaque,
 {
     int fd;
     Monitor *mon = cur_mon;
-    MonFdset *mon_fdset;
+    MonFdset *mon_fdset = NULL;
     MonFdsetFd *mon_fdset_fd;
     AddfdInfo *fdinfo;
 
@@ -2147,34 +2147,54 @@ AddfdInfo *qmp_add_fd(bool has_fdset_id, int64_t fdset_id, bool has_opaque,
 
     if (has_fdset_id) {
         QLIST_FOREACH(mon_fdset, &mon_fdsets, next) {
-            if (mon_fdset->id == fdset_id) {
+            /* Break if match found or match impossible due to ordering by ID */
+            if (fdset_id <= mon_fdset->id) {
+                if (fdset_id < mon_fdset->id) {
+                    mon_fdset = NULL;
+                }
                 break;
             }
         }
-        if (mon_fdset == NULL) {
-            error_set(errp, QERR_INVALID_PARAMETER_VALUE, "fdset-id",
-                      "an existing fdset-id");
-            goto error;
-        }
-    } else {
+    }
+
+    if (mon_fdset == NULL) {
         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;
+        if (has_fdset_id) {
+            if (fdset_id < 0) {
+                error_set(errp, QERR_INVALID_PARAMETER_VALUE, "fdset-id",
+                          "a non-negative value");
+                goto error;
+            }
+            /* Use specified fdset ID */
+            QLIST_FOREACH(mon_fdset, &mon_fdsets, next) {
+                mon_fdset_cur = mon_fdset;
+                if (fdset_id < mon_fdset_cur->id) {
+                    break;
+                }
+            }
+        } else {
+            /* 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;
             }
-            break;
         }
 
         mon_fdset = g_malloc0(sizeof(*mon_fdset));
-        mon_fdset->id = fdset_id_prev + 1;
+        if (has_fdset_id) {
+            mon_fdset->id = fdset_id;
+        } else {
+            mon_fdset->id = fdset_id_prev + 1;
+        }
 
         /* The fdset list is ordered by fdset ID */
-        if (mon_fdset->id == 0) {
+        if (!mon_fdset_cur) {
             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);
diff --git a/qapi-schema.json b/qapi-schema.json
index f9dbdae..12cdb74 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -2611,7 +2611,7 @@
 #
 # Returns: @AddfdInfo on success
 #          If file descriptor was not received, FdNotSupplied
-#          If @fdset-id does not exist, InvalidParameterValue
+#          If @fdset-id is a negative value, InvalidParameterValue
 #
 # Notes: The list of fd sets is shared by all monitor connections.
 #
-- 
1.7.11.4

[Qemu-devel] [PATCH v4 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.

Note: All of the code added to monitor_fdset_add_fd(), with the
exception of the error path for non-valid fdset-id, is code motion
from qmp_add_fd().

Signed-off-by: Corey Bryant <coreyb@linux.vnet.ibm.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
v2:
  -Removed Error** parameter from monitor_fdset_add_fd()

v3:
  -Added Error** parameter back to monitor_fdset_add_fd()
  -Move 'if (!mon_fdset_cur)' change to patch 1 (kwolf@redhat.com)
  -Move code that prevents removal of fd from fd set during init
   to it's own patch (eblake@redhat.com, kwolf@redhat.com)
  -Mention code motion in commit message (kwolf@redhat.com)

v4:
  -Fix fd leak in qmp_add_fd() (kwolf@redhat.com)

 monitor.c | 157 ++++++++++++++++++++++++++++++++++----------------------------
 monitor.h |   3 ++
 2 files changed, 88 insertions(+), 72 deletions(-)

diff --git a/monitor.c b/monitor.c
index 2e3248f..d8f66ca 100644
--- a/monitor.c
+++ b/monitor.c
@@ -2135,8 +2135,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 = NULL;
-    MonFdsetFd *mon_fdset_fd;
     AddfdInfo *fdinfo;
 
     fd = qemu_chr_fe_get_msgfd(mon->chr);
@@ -2145,78 +2143,12 @@ 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) {
-            /* Break if match found or match impossible due to ordering by ID */
-            if (fdset_id <= mon_fdset->id) {
-                if (fdset_id < mon_fdset->id) {
-                    mon_fdset = NULL;
-                }
-                break;
-            }
-        }
+    fdinfo = monitor_fdset_add_fd(fd, has_fdset_id, fdset_id,
+                                  has_opaque, opaque, errp);
+    if (fdinfo) {
+        return fdinfo;
     }
 
-    if (mon_fdset == NULL) {
-        int64_t fdset_id_prev = -1;
-        MonFdset *mon_fdset_cur = QLIST_FIRST(&mon_fdsets);
-
-        if (has_fdset_id) {
-            if (fdset_id < 0) {
-                error_set(errp, QERR_INVALID_PARAMETER_VALUE, "fdset-id",
-                          "a non-negative value");
-                goto error;
-            }
-            /* Use specified fdset ID */
-            QLIST_FOREACH(mon_fdset, &mon_fdsets, next) {
-                mon_fdset_cur = mon_fdset;
-                if (fdset_id < mon_fdset_cur->id) {
-                    break;
-                }
-            }
-        } else {
-            /* 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));
-        if (has_fdset_id) {
-            mon_fdset->id = fdset_id;
-        } else {
-            mon_fdset->id = fdset_id_prev + 1;
-        }
-
-        /* The fdset list is ordered by fdset ID */
-        if (!mon_fdset_cur) {
-            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:
     if (fd != -1) {
         close(fd);
@@ -2301,6 +2233,87 @@ 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 = NULL;
+    MonFdsetFd *mon_fdset_fd;
+    AddfdInfo *fdinfo;
+
+    if (has_fdset_id) {
+        QLIST_FOREACH(mon_fdset, &mon_fdsets, next) {
+            /* Break if match found or match impossible due to ordering by ID */
+            if (fdset_id <= mon_fdset->id) {
+                if (fdset_id < mon_fdset->id) {
+                    mon_fdset = NULL;
+                }
+                break;
+            }
+        }
+    }
+
+    if (mon_fdset == NULL) {
+        int64_t fdset_id_prev = -1;
+        MonFdset *mon_fdset_cur = QLIST_FIRST(&mon_fdsets);
+
+        if (has_fdset_id) {
+            if (fdset_id < 0) {
+                error_set(errp, QERR_INVALID_PARAMETER_VALUE, "fdset-id",
+                          "a non-negative value");
+                return NULL;
+            }
+            /* Use specified fdset ID */
+            QLIST_FOREACH(mon_fdset, &mon_fdsets, next) {
+                mon_fdset_cur = mon_fdset;
+                if (fdset_id < mon_fdset_cur->id) {
+                    break;
+                }
+            }
+        } else {
+            /* 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));
+        if (has_fdset_id) {
+            mon_fdset->id = fdset_id;
+        } else {
+            mon_fdset->id = fdset_id_prev + 1;
+        }
+
+        /* The fdset list is ordered by fdset ID */
+        if (!mon_fdset_cur) {
+            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;
+}
+
 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 v4 3/4] monitor: Prevent removing fd from set during init

[Corey Bryant]

If an fd is added to an fd set via the command line, and it is not
referenced by another command line option (ie. -drive), then clean
it up after QEMU initialization is complete.

Signed-off-by: Corey Bryant <coreyb@linux.vnet.ibm.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
v3:
  - This patch was split into it's own patch in v3
    (eblake@redhat.com, kwolf@redhat.com)

v4:
  - No changes

 monitor.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/monitor.c b/monitor.c
index d8f66ca..928e3ae 100644
--- a/monitor.c
+++ b/monitor.c
@@ -2105,8 +2105,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);
-- 
1.7.11.4

[Qemu-devel] [PATCH v4 4/4] qemu-config: Add new -add-fd command line option

[Corey Bryant]

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 3 and 4, 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)

v4
  - Update fd numbers in commit message text (eblake@redhat.com)
  - Handle corner case of -add-fd using an fd that is already
    in use by QEMU (eblake@redhat.com)

 qemu-config.c   | 22 ++++++++++++++++
 qemu-options.hx | 36 ++++++++++++++++++++++++++
 vl.c            | 78 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 136 insertions(+)

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..47095a2 100644
--- a/vl.c
+++ b/vl.c
@@ -790,6 +790,71 @@ 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 (fcntl(fd, F_GETFD) & FD_CLOEXEC) {
+        qerror_report(ERROR_CLASS_GENERIC_ERROR,
+                      "fd is not valid or already in use");
+        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 +3374,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 +3390,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);
-- 
1.7.11.4

Re: [Qemu-devel] [PATCH v4 4/4] qemu-config: Add new -add-fd command line option

[Eric Blake]

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

On 10/18/2012 01:19 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 3 and 4, 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>

> +
> +    if (fcntl(fd, F_GETFD) & FD_CLOEXEC) {
> +        qerror_report(ERROR_CLASS_GENERIC_ERROR,
> +                      "fd is not valid or already in use");
> +        return -1;
> +    }

Hmm, I was about to call you on the fact that you didn't check whether
fcntl() succeeded; but then realized that in the failure case it is
required by POSIX to return -1 which happens to include the FD_CLOEXEC
bit, so you actually ended up with a sneaky optimization that does the
right thing for both open and closed fds.

Perhaps a comment in the code is warranted (after all, it is not
immediately apparent from reading just this if statement why it works);
maybe "/* All fds inherited across exec() necessarily have FD_CLOEXEC
clear, while qemu sets FD_CLOEXEC on all other fds opened from command
line arguments */".  But I'm not going to require a v5 just for a
comment addition.

Series:
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
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 v4 4/4] qemu-config: Add new -add-fd command line option

[Corey Bryant]

On 10/18/2012 04:43 PM, Eric Blake wrote:
> On 10/18/2012 01:19 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 3 and 4, 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>
>
>> +
>> +    if (fcntl(fd, F_GETFD) & FD_CLOEXEC) {
>> +        qerror_report(ERROR_CLASS_GENERIC_ERROR,
>> +                      "fd is not valid or already in use");
>> +        return -1;
>> +    }
>
> Hmm, I was about to call you on the fact that you didn't check whether
> fcntl() succeeded; but then realized that in the failure case it is
> required by POSIX to return -1 which happens to include the FD_CLOEXEC
> bit, so you actually ended up with a sneaky optimization that does the
> right thing for both open and closed fds.

Yep it works for both cases.  I have to admit I stumbled into this at 
first and then decided to leave it this way since it worked. :)

>
> Perhaps a comment in the code is warranted (after all, it is not
> immediately apparent from reading just this if statement why it works);
> maybe "/* All fds inherited across exec() necessarily have FD_CLOEXEC
> clear, while qemu sets FD_CLOEXEC on all other fds opened from command
> line arguments */".  But I'm not going to require a v5 just for a
> comment addition.

I agree, a comment would be useful.  Maybe Kevin can add if this series 
gets pushed?

>
> Series:
> Reviewed-by: Eric Blake <eblake@redhat.com>
>

Thanks for reviewing!

-- 
Regards,
Corey Bryant

Re: [Qemu-devel] [PATCH v4 4/4] qemu-config: Add new -add-fd command line option

[Kevin Wolf]

Am 18.10.2012 23:37, schrieb Corey Bryant:
> 
> 
> On 10/18/2012 04:43 PM, Eric Blake wrote:
>> On 10/18/2012 01:19 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 3 and 4, 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>
>>
>>> +
>>> +    if (fcntl(fd, F_GETFD) & FD_CLOEXEC) {
>>> +        qerror_report(ERROR_CLASS_GENERIC_ERROR,
>>> +                      "fd is not valid or already in use");
>>> +        return -1;
>>> +    }
>>
>> Hmm, I was about to call you on the fact that you didn't check whether
>> fcntl() succeeded; but then realized that in the failure case it is
>> required by POSIX to return -1 which happens to include the FD_CLOEXEC
>> bit, so you actually ended up with a sneaky optimization that does the
>> right thing for both open and closed fds.
> 
> Yep it works for both cases.  I have to admit I stumbled into this at 
> first and then decided to leave it this way since it worked. :)

I wouldn't be surprised to find such subtleties in Fabrice's code, but
I'm not sure if adding new instances is the best idea ever. :-)

>> Perhaps a comment in the code is warranted (after all, it is not
>> immediately apparent from reading just this if statement why it works);
>> maybe "/* All fds inherited across exec() necessarily have FD_CLOEXEC
>> clear, while qemu sets FD_CLOEXEC on all other fds opened from command
>> line arguments */".  But I'm not going to require a v5 just for a
>> comment addition.
> 
> I agree, a comment would be useful.  Maybe Kevin can add if this series 
> gets pushed?

I'll amend the following to this patch, hope you both agree with the change:

diff --git a/vl.c b/vl.c
index 47095a2..5fb40da 100644
--- a/vl.c
+++ b/vl.c
@@ -792,7 +792,7 @@ static int parse_sandbox(QemuOpts *opts, void *opaque)

 static int parse_add_fd(QemuOpts *opts, void *opaque)
 {
-    int fd, dupfd;
+    int fd, dupfd, flags;
     int64_t fdset_id;
     const char *fd_opaque = NULL;

@@ -812,7 +812,12 @@ static int parse_add_fd(QemuOpts *opts, void *opaque)
         return -1;
     }

-    if (fcntl(fd, F_GETFD) & FD_CLOEXEC) {
+    /*
+     * All fds inherited across exec() necessarily have FD_CLOEXEC
+     * clear, while qemu sets FD_CLOEXEC on all other fds used internally.
+     */
+    flags = fcntl(fd, F_GETFD);
+    if (flags == -1 || (flags & FD_CLOEXEC)) {
         qerror_report(ERROR_CLASS_GENERIC_ERROR,
                       "fd is not valid or already in use");
         return -1;

Re: [Qemu-devel] [PATCH v4 4/4] qemu-config: Add new -add-fd command line option

[Corey Bryant]

On 10/19/2012 07:05 AM, Kevin Wolf wrote:
> Am 18.10.2012 23:37, schrieb Corey Bryant:
>>
>>
>> On 10/18/2012 04:43 PM, Eric Blake wrote:
>>> On 10/18/2012 01:19 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 3 and 4, 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>
>>>
>>>> +
>>>> +    if (fcntl(fd, F_GETFD) & FD_CLOEXEC) {
>>>> +        qerror_report(ERROR_CLASS_GENERIC_ERROR,
>>>> +                      "fd is not valid or already in use");
>>>> +        return -1;
>>>> +    }
>>>
>>> Hmm, I was about to call you on the fact that you didn't check whether
>>> fcntl() succeeded; but then realized that in the failure case it is
>>> required by POSIX to return -1 which happens to include the FD_CLOEXEC
>>> bit, so you actually ended up with a sneaky optimization that does the
>>> right thing for both open and closed fds.
>>
>> Yep it works for both cases.  I have to admit I stumbled into this at
>> first and then decided to leave it this way since it worked. :)
>
> I wouldn't be surprised to find such subtleties in Fabrice's code, but
> I'm not sure if adding new instances is the best idea ever. :-)
>
>>> Perhaps a comment in the code is warranted (after all, it is not
>>> immediately apparent from reading just this if statement why it works);
>>> maybe "/* All fds inherited across exec() necessarily have FD_CLOEXEC
>>> clear, while qemu sets FD_CLOEXEC on all other fds opened from command
>>> line arguments */".  But I'm not going to require a v5 just for a
>>> comment addition.
>>
>> I agree, a comment would be useful.  Maybe Kevin can add if this series
>> gets pushed?
>
> I'll amend the following to this patch, hope you both agree with the change:
>
> diff --git a/vl.c b/vl.c
> index 47095a2..5fb40da 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -792,7 +792,7 @@ static int parse_sandbox(QemuOpts *opts, void *opaque)
>
>   static int parse_add_fd(QemuOpts *opts, void *opaque)
>   {
> -    int fd, dupfd;
> +    int fd, dupfd, flags;
>       int64_t fdset_id;
>       const char *fd_opaque = NULL;
>
> @@ -812,7 +812,12 @@ static int parse_add_fd(QemuOpts *opts, void *opaque)
>           return -1;
>       }
>
> -    if (fcntl(fd, F_GETFD) & FD_CLOEXEC) {
> +    /*
> +     * All fds inherited across exec() necessarily have FD_CLOEXEC
> +     * clear, while qemu sets FD_CLOEXEC on all other fds used internally.
> +     */
> +    flags = fcntl(fd, F_GETFD);
> +    if (flags == -1 || (flags & FD_CLOEXEC)) {
>           qerror_report(ERROR_CLASS_GENERIC_ERROR,
>                         "fd is not valid or already in use");
>           return -1;
>

That works for me.  Thanks Kevin!

-- 
Regards,
Corey Bryant

Re: [Qemu-devel] [PATCH v4 4/4] qemu-config: Add new -add-fd command line option

[Eric Blake]

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

On 10/18/2012 01:19 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.
> 
> +
> +static int cleanup_add_fd(QemuOpts *opts, void *opaque)
> +{
> +    int fd;
> +
> +    fd = qemu_opt_get_number(opts, "fd", -1);
> +    close(fd);

One other subtle point: Given 'qemu-kvm -add-fd fd=3,set=1 -add-fd
fd=3,set=2', this code will call close(3) twice.  In a single-threaded
scenario, we happen to be safe (because we merely ignore that the second
one fails with EBADF), but in a multi-threaded scenario, it is a recipe
for disaster with a chance for closing an fd opened by another thread.

> @@ -3320,6 +3390,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);
> +    }

Thankfully, we only ever call that code in main(), prior to spawning
threads.  So my positive review still stands.

-- 
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 --]

[Qemu-devel] [PATCH v5] qemu-config: Add new -add-fd command line option

[Kevin Wolf]

From: Corey Bryant <coreyb@linux.vnet.ibm.com>

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 3 and 4, 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>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---

Sorry, Corey, hope you're okay with me taking over your patch... Your patch was
against the unmodified version while I already did some changes after the v4
review, so it didn't apply.

This version just completely disables fd passing on Windows as I don't think
it works there anyway. Gives you a nice error message instead of a silently
ignored -add-fd option.

Also added the missing break for case QEMU_OPTION_add_fd.

 qemu-config.c   |   22 +++++++++++++
 qemu-options.hx |   36 +++++++++++++++++++++
 vl.c            |   94 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 152 insertions(+), 0 deletions(-)

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 46f0539..a67a255 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -253,6 +253,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
@@ -285,6 +293,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 ee3c43a..b870caf 100644
--- a/vl.c
+++ b/vl.c
@@ -790,6 +790,78 @@ static int parse_sandbox(QemuOpts *opts, void *opaque)
     return 0;
 }
 
+#ifndef _WIN32
+static int parse_add_fd(QemuOpts *opts, void *opaque)
+{
+    int fd, dupfd, flags;
+    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;
+    }
+
+    /*
+     * All fds inherited across exec() necessarily have FD_CLOEXEC
+     * clear, while qemu sets FD_CLOEXEC on all other fds used internally.
+     */
+    flags = fcntl(fd, F_GETFD);
+    if (flags == -1 || (flags & FD_CLOEXEC)) {
+        qerror_report(ERROR_CLASS_GENERIC_ERROR,
+                      "fd is not valid or already in use");
+        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;
+}
+#endif
+
 /***********************************************************/
 /* QEMU Block devices */
 
@@ -3309,6 +3381,18 @@ int main(int argc, char **argv, char **envp)
                     exit(0);
                 }
                 break;
+            case QEMU_OPTION_add_fd:
+#ifndef _WIN32
+                opts = qemu_opts_parse(qemu_find_opts("add-fd"), optarg, 0);
+                if (!opts) {
+                    exit(0);
+                }
+#else
+                error_report("File descriptor passing is disabled on this "
+                             "platform");
+                exit(1);
+#endif
+                break;
             default:
                 os_parse_cmd_args(popt->index, optarg);
             }
@@ -3320,6 +3404,16 @@ int main(int argc, char **argv, char **envp)
         exit(1);
     }
 
+#ifndef _WIN32
+    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);
+    }
+#endif
+
     if (machine == NULL) {
         fprintf(stderr, "No machine found.\n");
         exit(1);
-- 
1.7.6.5

Re: [Qemu-devel] [PATCH v5] qemu-config: Add new -add-fd command line option

[Corey Bryant]

On 10/22/2012 10:36 AM, Kevin Wolf wrote:
> From: Corey Bryant <coreyb@linux.vnet.ibm.com>
>
> 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 3 and 4, 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>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>
> Sorry, Corey, hope you're okay with me taking over your patch... Your patch was
> against the unmodified version while I already did some changes after the v4
> review, so it didn't apply.
>

That's fine.  Thanks for the hand.

> This version just completely disables fd passing on Windows as I don't think
> it works there anyway. Gives you a nice error message instead of a silently
> ignored -add-fd option.
>
> Also added the missing break for case QEMU_OPTION_add_fd.
>
>   qemu-config.c   |   22 +++++++++++++
>   qemu-options.hx |   36 +++++++++++++++++++++
>   vl.c            |   94 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 152 insertions(+), 0 deletions(-)
>
> 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 46f0539..a67a255 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -253,6 +253,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
> @@ -285,6 +293,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 ee3c43a..b870caf 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -790,6 +790,78 @@ static int parse_sandbox(QemuOpts *opts, void *opaque)
>       return 0;
>   }
>
> +#ifndef _WIN32
> +static int parse_add_fd(QemuOpts *opts, void *opaque)
> +{
> +    int fd, dupfd, flags;
> +    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;
> +    }
> +
> +    /*
> +     * All fds inherited across exec() necessarily have FD_CLOEXEC
> +     * clear, while qemu sets FD_CLOEXEC on all other fds used internally.
> +     */
> +    flags = fcntl(fd, F_GETFD);
> +    if (flags == -1 || (flags & FD_CLOEXEC)) {
> +        qerror_report(ERROR_CLASS_GENERIC_ERROR,
> +                      "fd is not valid or already in use");
> +        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;
> +}
> +#endif
> +
>   /***********************************************************/
>   /* QEMU Block devices */
>
> @@ -3309,6 +3381,18 @@ int main(int argc, char **argv, char **envp)
>                       exit(0);
>                   }
>                   break;
> +            case QEMU_OPTION_add_fd:
> +#ifndef _WIN32
> +                opts = qemu_opts_parse(qemu_find_opts("add-fd"), optarg, 0);
> +                if (!opts) {
> +                    exit(0);
> +                }
> +#else
> +                error_report("File descriptor passing is disabled on this "
> +                             "platform");
> +                exit(1);
> +#endif
> +                break;
>               default:
>                   os_parse_cmd_args(popt->index, optarg);
>               }
> @@ -3320,6 +3404,16 @@ int main(int argc, char **argv, char **envp)
>           exit(1);
>       }
>
> +#ifndef _WIN32
> +    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);
> +    }
> +#endif
> +
>       if (machine == NULL) {
>           fprintf(stderr, "No machine found.\n");
>           exit(1);
>

This looks good to me.

-- 
Regards,
Corey Bryant