Patchwork [v2,2/3] monitor: Enable adding an inherited fd to an fd set

login
register
mail settings
Submitter Corey Bryant
Date Oct. 10, 2012, 2:20 p.m.
Message ID <1349878805-16352-3-git-send-email-coreyb@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/190648/
State New
Headers show

Comments

Corey Bryant - Oct. 10, 2012, 2:20 p.m.
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(-)
Eric Blake - Oct. 10, 2012, 10:01 p.m.
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]
Kevin Wolf - Oct. 11, 2012, 11:25 a.m.
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
Corey Bryant - Oct. 11, 2012, 2:30 p.m.
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.
Corey Bryant - Oct. 11, 2012, 3:04 p.m.
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
Kevin Wolf - Oct. 12, 2012, 8:30 a.m.
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

Patch

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);