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

[Qemu-devel] [PATCH v2 0/3] 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 (3):
  monitor: Allow add-fd to any specified fd set
  monitor: Enable adding an inherited fd to an fd set
  qemu-config: Add new -add-fd command line option

 monitor.c        | 131 +++++++++++++++++++++++++++++++------------------------
 monitor.h        |   2 +
 qapi-schema.json |   1 -
 qemu-config.c    |  22 ++++++++++
 qemu-options.hx  |  35 +++++++++++++++
 vl.c             |  36 +++++++++++++++
 6 files changed, 170 insertions(+), 57 deletions(-)

-- 
1.7.11.4

[Qemu-devel] [PATCH v2 1/3] 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>
---
v2:
 -This patch is new in v2

 monitor.c        | 41 ++++++++++++++++++++++++++---------------
 qapi-schema.json |  1 -
 2 files changed, 26 insertions(+), 16 deletions(-)

diff --git a/monitor.c b/monitor.c
index a0e3ffb..e53e733 100644
--- a/monitor.c
+++ b/monitor.c
@@ -2141,7 +2141,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;
 
@@ -2157,27 +2157,38 @@ AddfdInfo *qmp_add_fd(bool has_fdset_id, int64_t fdset_id, bool has_opaque,
                 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) {
+            /* 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) {
diff --git a/qapi-schema.json b/qapi-schema.json
index f9dbdae..06a7aa2 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -2611,7 +2611,6 @@
 #
 # Returns: @AddfdInfo on success
 #          If file descriptor was not received, FdNotSupplied
-#          If @fdset-id does not exist, InvalidParameterValue
 #
 # Notes: The list of fd sets is shared by all monitor connections.
 #
-- 
1.7.11.4

Re: [Qemu-devel] [PATCH v2 1/3] monitor: Allow add-fd to any specified fd set

[Eric Blake]

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

On 10/10/2012 08:20 AM, Corey Bryant wrote:
> 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.

Unfortunately, it now allows the user to choose arbitrary integer set
ids with large gaps, where previously, the user could only influence set
ids by populating all intermediate ids.  That is, before this patch, a
user would have to create 1000000 sets to have an id of 1000000 (if they
didn't run out of memory first on all the earlier sets), but now they
can have an id that large with just one set.  Or, taken further,
previously, a user request of -9223372036854775808 would likely fail (if
not, how beefy is your machine?), but now it can succeed and cause
confusion because of integer wraparound.  Arbitrary set ids is not
necessarily bad, but I think you need to add bounds-checking on the
user's requested fdset_id to make sure it is positive.

-- 
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 v2 1/3] monitor: Allow add-fd to any specified fd set

[Corey Bryant]

On 10/10/2012 05:49 PM, Eric Blake wrote:
> On 10/10/2012 08:20 AM, Corey Bryant wrote:
>> 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.
>
> Unfortunately, it now allows the user to choose arbitrary integer set
> ids with large gaps, where previously, the user could only influence set
> ids by populating all intermediate ids.  That is, before this patch, a
> user would have to create 1000000 sets to have an id of 1000000 (if they
> didn't run out of memory first on all the earlier sets), but now they
> can have an id that large with just one set.  Or, taken further,
> previously, a user request of -9223372036854775808 would likely fail (if
> not, how beefy is your machine?), but now it can succeed and cause
> confusion because of integer wraparound.  Arbitrary set ids is not
> necessarily bad, but I think you need to add bounds-checking on the
> user's requested fdset_id to make sure it is positive.
>

I agree.  I'll add some set ID bounds checking in v3.

-- 
Regards,
Corey Bryant

[Qemu-devel] [PATCH v2 2/3] 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>
---
v2:
  - Removed Error** parameter from monitor_fdset_add_fd()

 monitor.c | 142 +++++++++++++++++++++++++++++++++-----------------------------
 monitor.h |   2 +
 2 files changed, 77 insertions(+), 67 deletions(-)

diff --git a/monitor.c b/monitor.c
index e53e733..4226249 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,9 +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 = NULL;
-    MonFdsetFd *mon_fdset_fd;
-    AddfdInfo *fdinfo;
 
     fd = qemu_chr_fe_get_msgfd(mon->chr);
     if (fd == -1) {
@@ -2151,68 +2149,7 @@ 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) {
-        int64_t fdset_id_prev = -1;
-        MonFdset *mon_fdset_cur = QLIST_FIRST(&mon_fdsets);
-
-        if (has_fdset_id) {
-            /* 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->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;
+    return monitor_fdset_add_fd(fd, has_fdset_id, fdset_id, has_opaque, opaque);
 
 error:
     if (fd != -1) {
@@ -2298,6 +2235,77 @@ 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)
+{
+    MonFdset *mon_fdset = NULL;
+    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) {
+        int64_t fdset_id_prev = -1;
+        MonFdset *mon_fdset_cur = QLIST_FIRST(&mon_fdsets);
+
+        if (has_fdset_id) {
+            /* 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..78133d2 100644
--- a/monitor.h
+++ b/monitor.h
@@ -90,6 +90,8 @@ 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);
 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

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

[Eric Blake]

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

On 10/10/2012 08:20 AM, Corey Bryant wrote:
> 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.

You're mixing code motion and semantic change in one patch (keyword
"also" in your commit message; should the semantic change be moved to a
separate patch (possibly squashed into 1/3)?

> 
> Signed-off-by: Corey Bryant <coreyb@linux.vnet.ibm.com>
> ---

>  
> -    if (has_fdset_id) {
> -        QLIST_FOREACH(mon_fdset, &mon_fdsets, next) {
> -            if (mon_fdset->id == fdset_id) {
> -                break;
> -            }
> -        }
> -    }

Since you maintain mon_fdsets in sorted id order, should you optimize
this loop to abort the QLIST_FOREACH early if fdset_id is less than
mon_fdset->id?  [I only noticed this because of code motion, so it is a
pre-existing condition and therefore a separate patch, or another thing
to squash into 1/3]

-- 
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 v2 2/3] monitor: Enable adding an inherited fd to an fd set

[Corey Bryant]

On 10/10/2012 06:01 PM, Eric Blake wrote:
> On 10/10/2012 08:20 AM, Corey Bryant wrote:
>> 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.
>
> You're mixing code motion and semantic change in one patch (keyword
> "also" in your commit message; should the semantic change be moved to a
> separate patch (possibly squashed into 1/3)?
>

It should probably be another patch.  I'll look into this for v3.

>>
>> Signed-off-by: Corey Bryant <coreyb@linux.vnet.ibm.com>
>> ---
>
>>
>> -    if (has_fdset_id) {
>> -        QLIST_FOREACH(mon_fdset, &mon_fdsets, next) {
>> -            if (mon_fdset->id == fdset_id) {
>> -                break;
>> -            }
>> -        }
>> -    }
>
> Since you maintain mon_fdsets in sorted id order, should you optimize
> this loop to abort the QLIST_FOREACH early if fdset_id is less than
> mon_fdset->id?  [I only noticed this because of code motion, so it is a
> pre-existing condition and therefore a separate patch, or another thing
> to squash into 1/3]
>

Sure that makes sense to optimize this.

-- 
Regards,
Corey Bryant

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

[Kevin Wolf]

Am 10.10.2012 16:20, schrieb 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.

"This patch also..." usually means that it should be split in two
patches. Though in this case I'd vote for immediately dropping the
second patch again: This makes the probing work with file descriptors
using a hack for a certain situation (namely qemu startup) and leaves
other cases (like hotplug) broken.

I think it's kind of acceptable to leave it broken for both cases in
this patch series, you can work around it by passing the 'format'
option. If we do fix it, however, we should fix it consistently for all
use cases. The best approach for this is probably to avoid opening the
image file twice for probing; instead, we should first open the
BlockDriverState for raw-posix, use that for probing and then put a
second qcow2 or whatever BlockDriverState on top of the very same BDS
without reopening it.


The rest is code motion, except for:

-        if (mon_fdset->id == 0) {
+        if (!mon_fdset_cur) {

It's not nice to hide such changes in code motion patches, but the
change is obviously not wrong.

The commit message should be changed to tell that this is (mostly) code
motion. This doesn't really change anything semantically (except for the
probing part).

Kevin

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

[Corey Bryant]

On 10/11/2012 07:25 AM, Kevin Wolf wrote:
> Am 10.10.2012 16:20, schrieb 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.
>
> "This patch also..." usually means that it should be split in two
> patches. Though in this case I'd vote for immediately dropping the
> second patch again: This makes the probing work with file descriptors
> using a hack for a certain situation (namely qemu startup) and leaves
> other cases (like hotplug) broken.

I don't think hotplug is broken.  In that case the fd will only be 
removed from the fd set if the following is true:

(mon_fdset_fd->removed || (QLIST_EMPTY(&mon_fdset->dup_fds) && 
mon_refcount == 0))

We can ignore the removed part for now.  What's important here is that 
if there are no dup_fd references and there is at least one monitor 
connected, an fd will *not* be removed.

The problem with the command line -add-fd is that there is no equivalent 
to mon_refcount.  So I was using the check for runstate_is_running() as 
a (somewhat) equivalent to mon_refcount.  In other words, 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 or -blockdev), then clean it up 
after QEMU initialization.

>
> I think it's kind of acceptable to leave it broken for both cases in
> this patch series, you can work around it by passing the 'format'
> option. If we do fix it, however, we should fix it consistently for all
> use cases. The best approach for this is probably to avoid opening the
> image file twice for probing; instead, we should first open the
> BlockDriverState for raw-posix, use that for probing and then put a
> second qcow2 or whatever BlockDriverState on top of the very same BDS
> without reopening it.
>
>
> The rest is code motion, except for:
>
> -        if (mon_fdset->id == 0) {
> +        if (!mon_fdset_cur) {
>
> It's not nice to hide such changes in code motion patches, but the
> change is obviously not wrong.

Sorry about that.  I didn't mean to hide it.  This probably makes more 
sense in patch 1.

>
> The commit message should be changed to tell that this is (mostly) code
> motion. This doesn't really change anything semantically (except for the
> probing part).

Ok

-- 
Regards,
Corey Bryant

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

[Kevin Wolf]

Am 11.10.2012 17:04, schrieb Corey Bryant:
> 
> 
> On 10/11/2012 07:25 AM, Kevin Wolf wrote:
>> Am 10.10.2012 16:20, schrieb 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.
>>
>> "This patch also..." usually means that it should be split in two
>> patches. Though in this case I'd vote for immediately dropping the
>> second patch again: This makes the probing work with file descriptors
>> using a hack for a certain situation (namely qemu startup) and leaves
>> other cases (like hotplug) broken.
> 
> I don't think hotplug is broken.  In that case the fd will only be 
> removed from the fd set if the following is true:
> 
> (mon_fdset_fd->removed || (QLIST_EMPTY(&mon_fdset->dup_fds) && 
> mon_refcount == 0))
> 
> We can ignore the removed part for now.  What's important here is that 
> if there are no dup_fd references and there is at least one monitor 
> connected, an fd will *not* be removed.

Ah yes, that's the part I missed.

Then your approach of special-casing the command line is probably okay,
though I'd still want to change the probing mechanism to avoid the
reopen. Seems I need to find a better excuse to make someone do it. Meh. ;-)

Kevin

[Qemu-devel] [PATCH v2 3/3] 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=4,set=2,opaque="rdwr:/path/to/file"
             -add-fd fd=5,set=2,opaque="rdonly:/path/to/file"
             -drive file=/dev/fdset/2,index=0,media=disk

This example adds fds 4 and 5, and the accompanying opaque
strings to the fd set with ID=2.  qemu_open() already knows how
to handle a filename of this format.  qemu_open() searches the
corresponding fd set for an fd and when it finds a match, QEMU
goes on to use a dup of that fd just like it would have used an
fd that it opened itself.

Signed-off-by: Corey Bryant <coreyb@linux.vnet.ibm.com>
---
v2:
 - The -add-fd option is new in v2 (eblake@redhat.com)

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

diff --git a/qemu-config.c b/qemu-config.c
index cd1ec21..0bd67ca 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 to add 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..884fcb6 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=4,set=2,opaque="rdwr:/path/to/file"
+-add-fd fd=5,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,33 @@ 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 that is to be added to the fd set.
+@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=4,set=2,opaque="rdwr:/path/to/file"
+-add-fd fd=5,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 8d305ca..0265712 100644
--- a/vl.c
+++ b/vl.c
@@ -790,6 +790,33 @@ static int parse_sandbox(QemuOpts *opts, void *opaque)
     return 0;
 }
 
+static int parse_add_fd(QemuOpts *opts, void *opaque)
+{
+    int fd;
+    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 == -1) {
+        qerror_report(ERROR_CLASS_GENERIC_ERROR, "fd option is required");
+        return -1;
+    }
+
+    if (fdset_id == -1) {
+        qerror_report(ERROR_CLASS_GENERIC_ERROR, "set option is required");
+        return -1;
+    }
+
+    /* add the fd, and optionally the opaque string, to the fd set */
+    monitor_fdset_add_fd(fd, true, fdset_id, fd_opaque ? true : false,
+                         fd_opaque);
+
+    return 0;
+}
+
 /***********************************************************/
 /* QEMU Block devices */
 
@@ -3299,6 +3326,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);
             }
@@ -3310,6 +3342,10 @@ 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 (machine == NULL) {
         fprintf(stderr, "No machine found.\n");
         exit(1);
-- 
1.7.11.4

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

[Eric Blake]

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

On 10/10/2012 08:20 AM, 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=4,set=2,opaque="rdwr:/path/to/file"
>              -add-fd fd=5,set=2,opaque="rdonly:/path/to/file"
>              -drive file=/dev/fdset/2,index=0,media=disk
> 
> This example adds fds 4 and 5, and the accompanying opaque
> strings to the fd set with ID=2.  qemu_open() already knows how
> to handle a filename of this format.  qemu_open() searches the
> corresponding fd set for an fd and when it finds a match, QEMU
> goes on to use a dup of that fd just like it would have used an
> fd that it opened itself.

Missing some argument validation.  Earlier, I complained about set
validation, now I'm going to complain about fd validation.

> +static int parse_add_fd(QemuOpts *opts, void *opaque)
> +{
> +    int fd;
> +    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 I call 'qemu -add-fd fd=-2,set=1', that had better fail because fd -2
does not exist.  Likewise if I call 'qemu -add-fd fd=10000000,set=1'
(here, picking an fd that I know can't be opened).  More subtly, 'qemu
-add-fd fd=4,set=1 4<&-' should fail, since fd 4 was not inherited down
to the qemu process.  Fuzzier is whether 'qemu -add-fd fd=0,set=1' makes
sense, as that would then make stdin be competing for multiple uses;
this would be a situation that the monitor command can't duplicate, so
you have introduced new ways to possibly break things from the command
line.  I'm thinking it's safest to reject fd of 0, 1, or 2 for now, and
only relax it later IF we can prove that it would be both useful and safe.

So it sounds like you need something like fcntl(fd,F_GETFL) to see that
an the fd was actually inherited, as well as validating that fd >
STDERR_FILENO.

Another missing validation check is for duplicate use.  With the monitor
command, you ALWAYS have a unique fd (thanks to SCM_RIGHTS).  But with
the command line, I can type 'qemu -add-fd fd=4,set=1 -add-fd
fd=4,set=2'.  Oops - I've now corrupted your set layout, unless you
validate that every fd requested in -add-fd does not already reside in
any existing set.



Thinking aloud:
On the other hand, being able to pass in one fd to multiple sets MIGHT
be useful; in the SCM_RIGHTS monitor command case, I can pass the same
fd from the management perspective into multiple sets, even though in
qemu's perspective, there will be multiple fds created (one per call).
Perhaps instead of directly adding the inherited fd to a set, and having
to then sweep all sets to check for duplicates, it might make sense to
add dup(fd) to a set, so that if I call:

qemu -add-fd fd=4,set=1 -add-fd fd=4,set=2 -add-fd fd=5,set=2

what REALLY happens is that qemu adds dup(4)==6 to set 1, dup(4)==7 to
set 2, and dup(5)==8 to set 3.  Then, after all ALL -add-fd have been
processed, qemu then does another pass through them calling close(4) and
close(5) (to avoid holding the original fds open indefinitely if the
corresponding sets are discarded).  Hmm, notice that if you dup() before
adding to a set, then that the dup() resolves my first complaint of
validating that the inherited fd exists; it also changes the problem of
dealing with fd 0 (it would now be valid to add fd 0 to any number of
sets; but a final cleanup loop had better be careful to not call
close(0) unintentionally).

Personally, though, I'm not sure the complexity of using dup() buys us
anything, so I'm happy with the simpler solution of using fd as-is,
coupled with a check for no reuse of an fd already in a set.

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

[Corey Bryant]

On 10/10/2012 06:31 PM, Eric Blake wrote:
> On 10/10/2012 08:20 AM, 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=4,set=2,opaque="rdwr:/path/to/file"
>>               -add-fd fd=5,set=2,opaque="rdonly:/path/to/file"
>>               -drive file=/dev/fdset/2,index=0,media=disk
>>
>> This example adds fds 4 and 5, and the accompanying opaque
>> strings to the fd set with ID=2.  qemu_open() already knows how
>> to handle a filename of this format.  qemu_open() searches the
>> corresponding fd set for an fd and when it finds a match, QEMU
>> goes on to use a dup of that fd just like it would have used an
>> fd that it opened itself.
>
> Missing some argument validation.  Earlier, I complained about set
> validation, now I'm going to complain about fd validation.
>
>> +static int parse_add_fd(QemuOpts *opts, void *opaque)
>> +{
>> +    int fd;
>> +    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 I call 'qemu -add-fd fd=-2,set=1', that had better fail because fd -2
> does not exist.  Likewise if I call 'qemu -add-fd fd=10000000,set=1'
> (here, picking an fd that I know can't be opened).  More subtly, 'qemu
> -add-fd fd=4,set=1 4<&-' should fail, since fd 4 was not inherited down
> to the qemu process.  Fuzzier is whether 'qemu -add-fd fd=0,set=1' makes
> sense, as that would then make stdin be competing for multiple uses;
> this would be a situation that the monitor command can't duplicate, so
> you have introduced new ways to possibly break things from the command
> line.  I'm thinking it's safest to reject fd of 0, 1, or 2 for now, and
> only relax it later IF we can prove that it would be both useful and safe.
>
> So it sounds like you need something like fcntl(fd,F_GETFL) to see that
> an the fd was actually inherited, as well as validating that fd >
> STDERR_FILENO.

I agree.  I'll try this approach.

>
> Another missing validation check is for duplicate use.  With the monitor
> command, you ALWAYS have a unique fd (thanks to SCM_RIGHTS).  But with
> the command line, I can type 'qemu -add-fd fd=4,set=1 -add-fd
> fd=4,set=2'.  Oops - I've now corrupted your set layout, unless you
> validate that every fd requested in -add-fd does not already reside in
> any existing set.
>

I don't see this validation check for duplicate use of fd's being 
necessary.  Like you say below, in the QMP add-fd case we can add the 
same fd multiple times.  So we should be able to add the same fd 
multiple times via the command line.  The only difference between QMP 
and command line in this case is that the QMP fd is a dup and therefore 
a different number and the command line fd will be the same fd.  I'd 
prefer to leave this alone unless there's a compelling reason to block 
adding of the same fd.

>
>
> Thinking aloud:
> On the other hand, being able to pass in one fd to multiple sets MIGHT
> be useful; in the SCM_RIGHTS monitor command case, I can pass the same
> fd from the management perspective into multiple sets, even though in
> qemu's perspective, there will be multiple fds created (one per call).
> Perhaps instead of directly adding the inherited fd to a set, and having
> to then sweep all sets to check for duplicates, it might make sense to
> add dup(fd) to a set, so that if I call:
>
> qemu -add-fd fd=4,set=1 -add-fd fd=4,set=2 -add-fd fd=5,set=2
>
> what REALLY happens is that qemu adds dup(4)==6 to set 1, dup(4)==7 to
> set 2, and dup(5)==8 to set 3.  Then, after all ALL -add-fd have been
> processed, qemu then does another pass through them calling close(4) and
> close(5) (to avoid holding the original fds open indefinitely if the
> corresponding sets are discarded).  Hmm, notice that if you dup() before
> adding to a set, then that the dup() resolves my first complaint of
> validating that the inherited fd exists; it also changes the problem of
> dealing with fd 0 (it would now be valid to add fd 0 to any number of
> sets; but a final cleanup loop had better be careful to not call
> close(0) unintentionally).
>
> Personally, though, I'm not sure the complexity of using dup() buys us
> anything, so I'm happy with the simpler solution of using fd as-is,
> coupled with a check for no reuse of an fd already in a set.
>

-- 
Regards,
Corey Bryant

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

[Eric Blake]

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

On 10/11/2012 08:45 AM, Corey Bryant wrote:

>> Another missing validation check is for duplicate use.  With the monitor
>> command, you ALWAYS have a unique fd (thanks to SCM_RIGHTS).  But with
>> the command line, I can type 'qemu -add-fd fd=4,set=1 -add-fd
>> fd=4,set=2'.  Oops - I've now corrupted your set layout, unless you
>> validate that every fd requested in -add-fd does not already reside in
>> any existing set.
>>
> 
> I don't see this validation check for duplicate use of fd's being
> necessary.  Like you say below, in the QMP add-fd case we can add the
> same fd multiple times.  So we should be able to add the same fd
> multiple times via the command line.  The only difference between QMP
> and command line in this case is that the QMP fd is a dup and therefore
> a different number and the command line fd will be the same fd.  I'd
> prefer to leave this alone unless there's a compelling reason to block
> adding of the same fd.

There is a compelling reason to prevent duplicates among your sets:
qemu_close().

Suppose I add fd 4 into set 1 and 2, and then discard set 2 via monitor
commands.  Then, when qemu_close() drops the last reference to set 2, it
steps through and calls close() on all fds in that set, including fd 4.
 Oops - now set 1 is invalid, because it is tracking a closed fd.  And
worse, if qemu then does something else to open a new fd, it will get fd
4 again, and now set 1 will be tracking the WRONG fd.

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

[Corey Bryant]

On 10/11/2012 11:55 AM, Eric Blake wrote:
> On 10/11/2012 08:45 AM, Corey Bryant wrote:
>
>>> Another missing validation check is for duplicate use.  With the monitor
>>> command, you ALWAYS have a unique fd (thanks to SCM_RIGHTS).  But with
>>> the command line, I can type 'qemu -add-fd fd=4,set=1 -add-fd
>>> fd=4,set=2'.  Oops - I've now corrupted your set layout, unless you
>>> validate that every fd requested in -add-fd does not already reside in
>>> any existing set.
>>>
>>
>> I don't see this validation check for duplicate use of fd's being
>> necessary.  Like you say below, in the QMP add-fd case we can add the
>> same fd multiple times.  So we should be able to add the same fd
>> multiple times via the command line.  The only difference between QMP
>> and command line in this case is that the QMP fd is a dup and therefore
>> a different number and the command line fd will be the same fd.  I'd
>> prefer to leave this alone unless there's a compelling reason to block
>> adding of the same fd.
>
> There is a compelling reason to prevent duplicates among your sets:
> qemu_close().
>
> Suppose I add fd 4 into set 1 and 2, and then discard set 2 via monitor
> commands.  Then, when qemu_close() drops the last reference to set 2, it
> steps through and calls close() on all fds in that set, including fd 4.
>   Oops - now set 1 is invalid, because it is tracking a closed fd.  And
> worse, if qemu then does something else to open a new fd, it will get fd
> 4 again, and now set 1 will be tracking the WRONG fd.
>

Ah yes, that is compelling.  So we do need something here.  I'll reply 
to your other email regarding the approach to take.

-- 
Regards,
Corey Bryant

Re: [Qemu-devel] [libvirt] [PATCH v2 3/3] qemu-config: Add new -add-fd command line option

[Eric Blake]

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

On 10/10/2012 04:31 PM, Eric Blake wrote:

> Another missing validation check is for duplicate use.  With the monitor
> command, you ALWAYS have a unique fd (thanks to SCM_RIGHTS).  But with
> the command line, I can type 'qemu -add-fd fd=4,set=1 -add-fd
> fd=4,set=2'.  Oops - I've now corrupted your set layout, unless you
> validate that every fd requested in -add-fd does not already reside in
> any existing set.
> 
> Thinking aloud:
> On the other hand, being able to pass in one fd to multiple sets MIGHT
> be useful; in the SCM_RIGHTS monitor command case, I can pass the same
> fd from the management perspective into multiple sets, even though in
> qemu's perspective, there will be multiple fds created (one per call).
> Perhaps instead of directly adding the inherited fd to a set, and having
> to then sweep all sets to check for duplicates, it might make sense to
> add dup(fd) to a set, so that if I call:
> 
> qemu -add-fd fd=4,set=1 -add-fd fd=4,set=2 -add-fd fd=5,set=2
> 
> what REALLY happens is that qemu adds dup(4)==6 to set 1, dup(4)==7 to
> set 2, and dup(5)==8 to set 3.  Then, after all ALL -add-fd have been
> processed, qemu then does another pass through them calling close(4) and
> close(5) (to avoid holding the original fds open indefinitely if the
> corresponding sets are discarded).

Another idea: a hybrid approach - the _first_ -add-fd 4 directly adds 4
to the set, all other -add-fd 4 end up adding dup(4) instead (well,
fcntl(F_DUPFD_CLOEXEC), but you get the picture).  That is, do the
duplicate scanning, and if there is no duplicate, use the fd directly;
if there IS a duplicate, then put a unique fd number as a copy into the
remaining sets.  That way, you don't have to do a final close() sweep
across the -add-fd arguments passed on the command line, and you still
don't have to worry about duplicated fds across multiple sets causing
mayhem in qemu_close().

-- 
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] [libvirt] [PATCH v2 3/3] qemu-config: Add new -add-fd command line option

[Eric Blake]

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

On 10/11/2012 10:04 AM, Eric Blake wrote:
> Another idea: a hybrid approach - the _first_ -add-fd 4 directly adds 4
> to the set, all other -add-fd 4 end up adding dup(4) instead (well,
> fcntl(F_DUPFD_CLOEXEC), but you get the picture).  That is, do the
> duplicate scanning, and if there is no duplicate, use the fd directly;
> if there IS a duplicate, then put a unique fd number as a copy into the
> remaining sets.  That way, you don't have to do a final close() sweep
> across the -add-fd arguments passed on the command line, and you still
> don't have to worry about duplicated fds across multiple sets causing
> mayhem in qemu_close().

Hmm, you may also need to be careful of corner cases.  If I do:

qemu -add-fd fd=4,set=1 -add-fd fd=4,set=2 -add-fd fd=5,set=3 5<&-

with fd 5 not inherited, then a dup(4) would give 5; you don't want to
accidentally add a copy of fd 4 into set 3, but rather fail because fd 5
was not inherited.

-- 
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] [libvirt] [PATCH v2 3/3] qemu-config: Add new -add-fd command line option

[Corey Bryant]

On 10/11/2012 12:04 PM, Eric Blake wrote:
> On 10/10/2012 04:31 PM, Eric Blake wrote:
>
>> Another missing validation check is for duplicate use.  With the monitor
>> command, you ALWAYS have a unique fd (thanks to SCM_RIGHTS).  But with
>> the command line, I can type 'qemu -add-fd fd=4,set=1 -add-fd
>> fd=4,set=2'.  Oops - I've now corrupted your set layout, unless you
>> validate that every fd requested in -add-fd does not already reside in
>> any existing set.
>>
>> Thinking aloud:
>> On the other hand, being able to pass in one fd to multiple sets MIGHT
>> be useful; in the SCM_RIGHTS monitor command case, I can pass the same
>> fd from the management perspective into multiple sets, even though in
>> qemu's perspective, there will be multiple fds created (one per call).
>> Perhaps instead of directly adding the inherited fd to a set, and having
>> to then sweep all sets to check for duplicates, it might make sense to
>> add dup(fd) to a set, so that if I call:
>>
>> qemu -add-fd fd=4,set=1 -add-fd fd=4,set=2 -add-fd fd=5,set=2
>>
>> what REALLY happens is that qemu adds dup(4)==6 to set 1, dup(4)==7 to
>> set 2, and dup(5)==8 to set 3.  Then, after all ALL -add-fd have been
>> processed, qemu then does another pass through them calling close(4) and
>> close(5) (to avoid holding the original fds open indefinitely if the
>> corresponding sets are discarded).
>
> Another idea: a hybrid approach - the _first_ -add-fd 4 directly adds 4
> to the set, all other -add-fd 4 end up adding dup(4) instead (well,
> fcntl(F_DUPFD_CLOEXEC), but you get the picture).  That is, do the
> duplicate scanning, and if there is no duplicate, use the fd directly;
> if there IS a duplicate, then put a unique fd number as a copy into the
> remaining sets.  That way, you don't have to do a final close() sweep
> across the -add-fd arguments passed on the command line, and you still
> don't have to worry about duplicated fds across multiple sets causing
> mayhem in qemu_close().
>

This would simplify the code, but I wonder if it would be confusing to 
users when they call query-fdsets and only see a single fd 4.  It may 
make more sense to dup all fds that are passed with -add-fd, and then it 
basically works the same as the QMP add-fd via SCM_RIGHTS.

On a somewhat related note, one major difference between the QMP add-fd 
and  command line -add-fd, is that -add-fd doesn't return the fd that 
was added.  So opaque strings will be even more important when passing 
fds on the command line.

-- 
Regards,
Corey Bryant