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

[Qemu-devel] [PATCH v3 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>
---
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)

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

diff --git a/monitor.c b/monitor.c
index 2e3248f..5d5de41 100644
--- a/monitor.c
+++ b/monitor.c
@@ -2135,9 +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);
     if (fd == -1) {
@@ -2145,77 +2142,8 @@ 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;
-            }
-        }
-    }
-
-    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;
+    return monitor_fdset_add_fd(fd, has_fdset_id, fdset_id,
+                                has_opaque, opaque, errp);
 
 error:
     if (fd != -1) {
@@ -2301,6 +2229,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

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

[Eric Blake]

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

On 10/16/2012 12:08 PM, 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.
> 
> 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>
> ---
> 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)

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

[Kevin Wolf]

Am 16.10.2012 20:08, 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.
> 
> 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>

And the non-valid fdset-id error case leaks fd now, right?

Kevin

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

[Corey Bryant]

On 10/17/2012 09:45 AM, Kevin Wolf wrote:
> Am 16.10.2012 20:08, 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.
>>
>> 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>
>
> And the non-valid fdset-id error case leaks fd now, right?
>
> Kevin
>

Yes, I need to fix that.  Thanks.  I'll send out a v4.

-- 
Regards,
Corey Bryant