[Qemu-devel] [PATCH 0/2] avoid races on exec migration

[Qemu-devel] [PATCH 0/2] avoid races on exec migration

[Paolo Bonzini]

QEMU has a sigchld handler that reaps any child process.  -smb is the
only user of it and, in fact, QEMU inherited it from slirp.  However,
this handler causes 'exec' based migration to randomly return 'status:
failed' in the monitor.  This happens when the signal handler for SIGCHLD
is ran before the pclose() of exec migration.

The return status of fclose() is passed back as return status of
qemu_fclose().  If qemu_fclose() fails, then the exec_close() in
migration-exec.c returns a error code. This causes migrate_fd_cleanup()
to return an error, and thus finally we see why 'status: failed' occurs:

        if (migrate_fd_cleanup(s) < 0) {
            if (old_vm_running) {
                vm_start();
            }
            state = MIG_STATE_ERROR;
        }

To avoid this, register the pids in a list and, on SIGCHLD, set up a
bottom-half that would go through the pids and reap them.

Since I'm at it, I'm moving iohandler stuff out of vl.c.  The new
file isn't a perfect place to add the child watcher, but it's arguably
better than vl.c.

This should be applied to both master and stable.

Paolo Bonzini (2):
  extract I/O handler lists to iohandler.c
  add a service to reap zombies

 Makefile.objs |    2 +-
 iohandler.c   |  193 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 os-posix.c    |    9 ---
 qemu-common.h |    4 +
 slirp/misc.c  |    5 +-
 vl.c          |  106 ++------------------------------
 6 files changed, 207 insertions(+), 112 deletions(-)
 create mode 100644 iohandler.c

-- 
1.7.4

[Qemu-devel] [PATCH 1/2] extract I/O handler lists to iohandler.c

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
        I had this patch queued for a while; only today I noticed the
        very similar one in virtagent.

 Makefile.objs |    2 +-
 iohandler.c   |  129 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 qemu-common.h |    3 +
 vl.c          |  106 ++--------------------------------------------
 4 files changed, 138 insertions(+), 102 deletions(-)
 create mode 100644 iohandler.c

diff --git a/Makefile.objs b/Makefile.objs
index 9e98a66..f282507 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -98,7 +98,7 @@ common-obj-y += buffered_file.o migration.o migration-tcp.o qemu-sockets.o
 common-obj-y += qemu-char.o savevm.o #aio.o
 common-obj-y += msmouse.o ps2.o
 common-obj-y += qdev.o qdev-properties.o
-common-obj-y += block-migration.o
+common-obj-y += block-migration.o iohandler.o
 common-obj-y += pflib.o
 common-obj-y += bitmap.o bitops.o
 
diff --git a/iohandler.c b/iohandler.c
new file mode 100644
index 0000000..2e30fe3
--- /dev/null
+++ b/iohandler.c
@@ -0,0 +1,129 @@
+/*
+ * QEMU System Emulator - managing I/O handler
+ *
+ * Copyright (c) 2003-2008 Fabrice Bellard
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "config-host.h"
+#include "qemu-common.h"
+#include "qemu-char.h"
+#include "qemu-queue.h"
+
+typedef struct IOHandlerRecord {
+    int fd;
+    IOCanReadHandler *fd_read_poll;
+    IOHandler *fd_read;
+    IOHandler *fd_write;
+    int deleted;
+    void *opaque;
+    QLIST_ENTRY(IOHandlerRecord) next;
+} IOHandlerRecord;
+
+static QLIST_HEAD(, IOHandlerRecord) io_handlers =
+    QLIST_HEAD_INITIALIZER(io_handlers);
+
+
+/* XXX: fd_read_poll should be suppressed, but an API change is
+   necessary in the character devices to suppress fd_can_read(). */
+int qemu_set_fd_handler2(int fd,
+                         IOCanReadHandler *fd_read_poll,
+                         IOHandler *fd_read,
+                         IOHandler *fd_write,
+                         void *opaque)
+{
+    IOHandlerRecord *ioh;
+
+    if (!fd_read && !fd_write) {
+        QLIST_FOREACH(ioh, &io_handlers, next) {
+            if (ioh->fd == fd) {
+                ioh->deleted = 1;
+                break;
+            }
+        }
+    } else {
+        QLIST_FOREACH(ioh, &io_handlers, next) {
+            if (ioh->fd == fd)
+                goto found;
+        }
+        ioh = qemu_mallocz(sizeof(IOHandlerRecord));
+        QLIST_INSERT_HEAD(&io_handlers, ioh, next);
+    found:
+        ioh->fd = fd;
+        ioh->fd_read_poll = fd_read_poll;
+        ioh->fd_read = fd_read;
+        ioh->fd_write = fd_write;
+        ioh->opaque = opaque;
+        ioh->deleted = 0;
+    }
+    return 0;
+}
+
+int qemu_set_fd_handler(int fd,
+                        IOHandler *fd_read,
+                        IOHandler *fd_write,
+                        void *opaque)
+{
+    return qemu_set_fd_handler2(fd, NULL, fd_read, fd_write, opaque);
+}
+
+void qemu_iohandler_fill(int *pnfds, fd_set *readfds, fd_set *writefds, fd_set *xfds)
+{
+    IOHandlerRecord *ioh;
+
+    QLIST_FOREACH(ioh, &io_handlers, next) {
+        if (ioh->deleted)
+            continue;
+        if (ioh->fd_read &&
+            (!ioh->fd_read_poll ||
+             ioh->fd_read_poll(ioh->opaque) != 0)) {
+            FD_SET(ioh->fd, readfds);
+            if (ioh->fd > *pnfds)
+                *pnfds = ioh->fd;
+        }
+        if (ioh->fd_write) {
+            FD_SET(ioh->fd, writefds);
+            if (ioh->fd > *pnfds)
+                *pnfds = ioh->fd;
+        }
+    }
+}
+
+void qemu_iohandler_poll(fd_set *readfds, fd_set *writefds, fd_set *xfds, int ret)
+{
+    if (ret > 0) {
+        IOHandlerRecord *pioh, *ioh;
+
+        QLIST_FOREACH_SAFE(ioh, &io_handlers, next, pioh) {
+            if (!ioh->deleted && ioh->fd_read && FD_ISSET(ioh->fd, readfds)) {
+                ioh->fd_read(ioh->opaque);
+            }
+            if (!ioh->deleted && ioh->fd_write && FD_ISSET(ioh->fd, writefds)) {
+                ioh->fd_write(ioh->opaque);
+            }
+
+            /* Do this last in case read/write handlers marked it for deletion */
+            if (ioh->deleted) {
+                QLIST_REMOVE(ioh, next);
+                qemu_free(ioh);
+            }
+        }
+    }
+}
diff --git a/qemu-common.h b/qemu-common.h
index 40dad52..27855b0 100644
--- a/qemu-common.h
+++ b/qemu-common.h
@@ -223,6 +223,9 @@ typedef void IOReadHandler(void *opaque, const uint8_t *buf, int size);
 typedef int IOCanReadHandler(void *opaque);
 typedef void IOHandler(void *opaque);
 
+void qemu_iohandler_fill(int *pnfds, fd_set *readfds, fd_set *writefds, fd_set *xfds);
+void qemu_iohandler_poll(fd_set *readfds, fd_set *writefds, fd_set *xfds, int rc);
+
 struct ParallelIOArg {
     void *buffer;
     int count;
diff --git a/vl.c b/vl.c
index b436952..224a564 100644
--- a/vl.c
+++ b/vl.c
@@ -1023,68 +1023,6 @@ void pcmcia_info(Monitor *mon)
 }
 
 /***********************************************************/
-/* I/O handling */
-
-typedef struct IOHandlerRecord {
-    int fd;
-    IOCanReadHandler *fd_read_poll;
-    IOHandler *fd_read;
-    IOHandler *fd_write;
-    int deleted;
-    void *opaque;
-    /* temporary data */
-    struct pollfd *ufd;
-    QLIST_ENTRY(IOHandlerRecord) next;
-} IOHandlerRecord;
-
-static QLIST_HEAD(, IOHandlerRecord) io_handlers =
-    QLIST_HEAD_INITIALIZER(io_handlers);
-
-
-/* XXX: fd_read_poll should be suppressed, but an API change is
-   necessary in the character devices to suppress fd_can_read(). */
-int qemu_set_fd_handler2(int fd,
-                         IOCanReadHandler *fd_read_poll,
-                         IOHandler *fd_read,
-                         IOHandler *fd_write,
-                         void *opaque)
-{
-    IOHandlerRecord *ioh;
-
-    if (!fd_read && !fd_write) {
-        QLIST_FOREACH(ioh, &io_handlers, next) {
-            if (ioh->fd == fd) {
-                ioh->deleted = 1;
-                break;
-            }
-        }
-    } else {
-        QLIST_FOREACH(ioh, &io_handlers, next) {
-            if (ioh->fd == fd)
-                goto found;
-        }
-        ioh = qemu_mallocz(sizeof(IOHandlerRecord));
-        QLIST_INSERT_HEAD(&io_handlers, ioh, next);
-    found:
-        ioh->fd = fd;
-        ioh->fd_read_poll = fd_read_poll;
-        ioh->fd_read = fd_read;
-        ioh->fd_write = fd_write;
-        ioh->opaque = opaque;
-        ioh->deleted = 0;
-    }
-    return 0;
-}
-
-int qemu_set_fd_handler(int fd,
-                        IOHandler *fd_read,
-                        IOHandler *fd_write,
-                        void *opaque)
-{
-    return qemu_set_fd_handler2(fd, NULL, fd_read, fd_write, opaque);
-}
-
-/***********************************************************/
 /* machine registration */
 
 static QEMUMachine *first_machine = NULL;
@@ -1326,7 +1264,6 @@ void qemu_system_vmstop_request(int reason)
 
 void main_loop_wait(int nonblocking)
 {
-    IOHandlerRecord *ioh;
     fd_set rfds, wfds, xfds;
     int ret, nfds;
     struct timeval tv;
@@ -1341,56 +1278,23 @@ void main_loop_wait(int nonblocking)
 
     os_host_main_loop_wait(&timeout);
 
+    tv.tv_sec = timeout / 1000;
+    tv.tv_usec = (timeout % 1000) * 1000;
+
     /* poll any events */
     /* XXX: separate device handlers from system ones */
     nfds = -1;
     FD_ZERO(&rfds);
     FD_ZERO(&wfds);
     FD_ZERO(&xfds);
-    QLIST_FOREACH(ioh, &io_handlers, next) {
-        if (ioh->deleted)
-            continue;
-        if (ioh->fd_read &&
-            (!ioh->fd_read_poll ||
-             ioh->fd_read_poll(ioh->opaque) != 0)) {
-            FD_SET(ioh->fd, &rfds);
-            if (ioh->fd > nfds)
-                nfds = ioh->fd;
-        }
-        if (ioh->fd_write) {
-            FD_SET(ioh->fd, &wfds);
-            if (ioh->fd > nfds)
-                nfds = ioh->fd;
-        }
-    }
-
-    tv.tv_sec = timeout / 1000;
-    tv.tv_usec = (timeout % 1000) * 1000;
-
+    qemu_iohandler_fill(&nfds, &rfds, &wfds, &xfds);
     slirp_select_fill(&nfds, &rfds, &wfds, &xfds);
 
     qemu_mutex_unlock_iothread();
     ret = select(nfds + 1, &rfds, &wfds, &xfds, &tv);
     qemu_mutex_lock_iothread();
-    if (ret > 0) {
-        IOHandlerRecord *pioh;
-
-        QLIST_FOREACH_SAFE(ioh, &io_handlers, next, pioh) {
-            if (!ioh->deleted && ioh->fd_read && FD_ISSET(ioh->fd, &rfds)) {
-                ioh->fd_read(ioh->opaque);
-            }
-            if (!ioh->deleted && ioh->fd_write && FD_ISSET(ioh->fd, &wfds)) {
-                ioh->fd_write(ioh->opaque);
-            }
-
-            /* Do this last in case read/write handlers marked it for deletion */
-            if (ioh->deleted) {
-                QLIST_REMOVE(ioh, next);
-                qemu_free(ioh);
-            }
-        }
-    }
 
+    qemu_iohandler_poll(&rfds, &wfds, &xfds, ret);
     slirp_select_poll(&rfds, &wfds, &xfds, (ret < 0));
 
     qemu_run_all_timers();
-- 
1.7.4

Re: [Qemu-devel] [PATCH 1/2] extract I/O handler lists to iohandler.c

[Roy Tam]

Hi,

2011/3/10 Paolo Bonzini <pbonzini@redhat.com>:
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>        I had this patch queued for a while; only today I noticed the
>        very similar one in virtagent.
>
>  Makefile.objs |    2 +-
>  iohandler.c   |  129 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  qemu-common.h |    3 +
>  vl.c          |  106 ++--------------------------------------------
>  4 files changed, 138 insertions(+), 102 deletions(-)
>  create mode 100644 iohandler.c
>
> diff --git a/Makefile.objs b/Makefile.objs
> index 9e98a66..f282507 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -98,7 +98,7 @@ common-obj-y += buffered_file.o migration.o migration-tcp.o qemu-sockets.o
>  common-obj-y += qemu-char.o savevm.o #aio.o
>  common-obj-y += msmouse.o ps2.o
>  common-obj-y += qdev.o qdev-properties.o
> -common-obj-y += block-migration.o
> +common-obj-y += block-migration.o iohandler.o
>  common-obj-y += pflib.o
>  common-obj-y += bitmap.o bitops.o
>
> diff --git a/iohandler.c b/iohandler.c
> new file mode 100644
> index 0000000..2e30fe3
> --- /dev/null
> +++ b/iohandler.c
> @@ -0,0 +1,129 @@
> +/*
> + * QEMU System Emulator - managing I/O handler
> + *
> + * Copyright (c) 2003-2008 Fabrice Bellard
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +#include "config-host.h"
> +#include "qemu-common.h"
> +#include "qemu-char.h"
> +#include "qemu-queue.h"
> +
> +typedef struct IOHandlerRecord {
> +    int fd;
> +    IOCanReadHandler *fd_read_poll;
> +    IOHandler *fd_read;
> +    IOHandler *fd_write;
> +    int deleted;
> +    void *opaque;
> +    QLIST_ENTRY(IOHandlerRecord) next;
> +} IOHandlerRecord;
> +
> +static QLIST_HEAD(, IOHandlerRecord) io_handlers =
> +    QLIST_HEAD_INITIALIZER(io_handlers);
> +
> +
> +/* XXX: fd_read_poll should be suppressed, but an API change is
> +   necessary in the character devices to suppress fd_can_read(). */
> +int qemu_set_fd_handler2(int fd,
> +                         IOCanReadHandler *fd_read_poll,
> +                         IOHandler *fd_read,
> +                         IOHandler *fd_write,
> +                         void *opaque)
> +{
> +    IOHandlerRecord *ioh;
> +
> +    if (!fd_read && !fd_write) {
> +        QLIST_FOREACH(ioh, &io_handlers, next) {
> +            if (ioh->fd == fd) {
> +                ioh->deleted = 1;
> +                break;
> +            }
> +        }
> +    } else {
> +        QLIST_FOREACH(ioh, &io_handlers, next) {
> +            if (ioh->fd == fd)
> +                goto found;
> +        }
> +        ioh = qemu_mallocz(sizeof(IOHandlerRecord));
> +        QLIST_INSERT_HEAD(&io_handlers, ioh, next);
> +    found:
> +        ioh->fd = fd;
> +        ioh->fd_read_poll = fd_read_poll;
> +        ioh->fd_read = fd_read;
> +        ioh->fd_write = fd_write;
> +        ioh->opaque = opaque;
> +        ioh->deleted = 0;
> +    }
> +    return 0;
> +}
> +
> +int qemu_set_fd_handler(int fd,
> +                        IOHandler *fd_read,
> +                        IOHandler *fd_write,
> +                        void *opaque)
> +{
> +    return qemu_set_fd_handler2(fd, NULL, fd_read, fd_write, opaque);
> +}
> +
> +void qemu_iohandler_fill(int *pnfds, fd_set *readfds, fd_set *writefds, fd_set *xfds)
> +{
> +    IOHandlerRecord *ioh;
> +
> +    QLIST_FOREACH(ioh, &io_handlers, next) {
> +        if (ioh->deleted)
> +            continue;
> +        if (ioh->fd_read &&
> +            (!ioh->fd_read_poll ||
> +             ioh->fd_read_poll(ioh->opaque) != 0)) {
> +            FD_SET(ioh->fd, readfds);
> +            if (ioh->fd > *pnfds)
> +                *pnfds = ioh->fd;
> +        }
> +        if (ioh->fd_write) {
> +            FD_SET(ioh->fd, writefds);
> +            if (ioh->fd > *pnfds)
> +                *pnfds = ioh->fd;
> +        }
> +    }
> +}
> +
> +void qemu_iohandler_poll(fd_set *readfds, fd_set *writefds, fd_set *xfds, int ret)
> +{
> +    if (ret > 0) {
> +        IOHandlerRecord *pioh, *ioh;
> +
> +        QLIST_FOREACH_SAFE(ioh, &io_handlers, next, pioh) {
> +            if (!ioh->deleted && ioh->fd_read && FD_ISSET(ioh->fd, readfds)) {
> +                ioh->fd_read(ioh->opaque);
> +            }
> +            if (!ioh->deleted && ioh->fd_write && FD_ISSET(ioh->fd, writefds)) {
> +                ioh->fd_write(ioh->opaque);
> +            }
> +
> +            /* Do this last in case read/write handlers marked it for deletion */
> +            if (ioh->deleted) {
> +                QLIST_REMOVE(ioh, next);
> +                qemu_free(ioh);
> +            }
> +        }
> +    }
> +}
> diff --git a/qemu-common.h b/qemu-common.h
> index 40dad52..27855b0 100644
> --- a/qemu-common.h
> +++ b/qemu-common.h
> @@ -223,6 +223,9 @@ typedef void IOReadHandler(void *opaque, const uint8_t *buf, int size);
>  typedef int IOCanReadHandler(void *opaque);
>  typedef void IOHandler(void *opaque);
>
> +void qemu_iohandler_fill(int *pnfds, fd_set *readfds, fd_set *writefds, fd_set *xfds);
> +void qemu_iohandler_poll(fd_set *readfds, fd_set *writefds, fd_set *xfds, int rc);
> +

This cause compilation error in MinGW:

  CC    qemu-img.o
In file included from qemu-img.c:24:
qemu-common.h:231: error: syntax error before "fd_set"
qemu-common.h:231: warning: function declaration isn't a prototype
qemu-common.h:232: error: syntax error before '*' token
qemu-common.h:232: warning: function declaration isn't a prototype
make: *** [qemu-img.o] Error 1


>  struct ParallelIOArg {
>     void *buffer;
>     int count;
> diff --git a/vl.c b/vl.c
> index b436952..224a564 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1023,68 +1023,6 @@ void pcmcia_info(Monitor *mon)
>  }
>
>  /***********************************************************/
> -/* I/O handling */
> -
> -typedef struct IOHandlerRecord {
> -    int fd;
> -    IOCanReadHandler *fd_read_poll;
> -    IOHandler *fd_read;
> -    IOHandler *fd_write;
> -    int deleted;
> -    void *opaque;
> -    /* temporary data */
> -    struct pollfd *ufd;
> -    QLIST_ENTRY(IOHandlerRecord) next;
> -} IOHandlerRecord;
> -
> -static QLIST_HEAD(, IOHandlerRecord) io_handlers =
> -    QLIST_HEAD_INITIALIZER(io_handlers);
> -
> -
> -/* XXX: fd_read_poll should be suppressed, but an API change is
> -   necessary in the character devices to suppress fd_can_read(). */
> -int qemu_set_fd_handler2(int fd,
> -                         IOCanReadHandler *fd_read_poll,
> -                         IOHandler *fd_read,
> -                         IOHandler *fd_write,
> -                         void *opaque)
> -{
> -    IOHandlerRecord *ioh;
> -
> -    if (!fd_read && !fd_write) {
> -        QLIST_FOREACH(ioh, &io_handlers, next) {
> -            if (ioh->fd == fd) {
> -                ioh->deleted = 1;
> -                break;
> -            }
> -        }
> -    } else {
> -        QLIST_FOREACH(ioh, &io_handlers, next) {
> -            if (ioh->fd == fd)
> -                goto found;
> -        }
> -        ioh = qemu_mallocz(sizeof(IOHandlerRecord));
> -        QLIST_INSERT_HEAD(&io_handlers, ioh, next);
> -    found:
> -        ioh->fd = fd;
> -        ioh->fd_read_poll = fd_read_poll;
> -        ioh->fd_read = fd_read;
> -        ioh->fd_write = fd_write;
> -        ioh->opaque = opaque;
> -        ioh->deleted = 0;
> -    }
> -    return 0;
> -}
> -
> -int qemu_set_fd_handler(int fd,
> -                        IOHandler *fd_read,
> -                        IOHandler *fd_write,
> -                        void *opaque)
> -{
> -    return qemu_set_fd_handler2(fd, NULL, fd_read, fd_write, opaque);
> -}
> -
> -/***********************************************************/
>  /* machine registration */
>
>  static QEMUMachine *first_machine = NULL;
> @@ -1326,7 +1264,6 @@ void qemu_system_vmstop_request(int reason)
>
>  void main_loop_wait(int nonblocking)
>  {
> -    IOHandlerRecord *ioh;
>     fd_set rfds, wfds, xfds;
>     int ret, nfds;
>     struct timeval tv;
> @@ -1341,56 +1278,23 @@ void main_loop_wait(int nonblocking)
>
>     os_host_main_loop_wait(&timeout);
>
> +    tv.tv_sec = timeout / 1000;
> +    tv.tv_usec = (timeout % 1000) * 1000;
> +
>     /* poll any events */
>     /* XXX: separate device handlers from system ones */
>     nfds = -1;
>     FD_ZERO(&rfds);
>     FD_ZERO(&wfds);
>     FD_ZERO(&xfds);
> -    QLIST_FOREACH(ioh, &io_handlers, next) {
> -        if (ioh->deleted)
> -            continue;
> -        if (ioh->fd_read &&
> -            (!ioh->fd_read_poll ||
> -             ioh->fd_read_poll(ioh->opaque) != 0)) {
> -            FD_SET(ioh->fd, &rfds);
> -            if (ioh->fd > nfds)
> -                nfds = ioh->fd;
> -        }
> -        if (ioh->fd_write) {
> -            FD_SET(ioh->fd, &wfds);
> -            if (ioh->fd > nfds)
> -                nfds = ioh->fd;
> -        }
> -    }
> -
> -    tv.tv_sec = timeout / 1000;
> -    tv.tv_usec = (timeout % 1000) * 1000;
> -
> +    qemu_iohandler_fill(&nfds, &rfds, &wfds, &xfds);
>     slirp_select_fill(&nfds, &rfds, &wfds, &xfds);
>
>     qemu_mutex_unlock_iothread();
>     ret = select(nfds + 1, &rfds, &wfds, &xfds, &tv);
>     qemu_mutex_lock_iothread();
> -    if (ret > 0) {
> -        IOHandlerRecord *pioh;
> -
> -        QLIST_FOREACH_SAFE(ioh, &io_handlers, next, pioh) {
> -            if (!ioh->deleted && ioh->fd_read && FD_ISSET(ioh->fd, &rfds)) {
> -                ioh->fd_read(ioh->opaque);
> -            }
> -            if (!ioh->deleted && ioh->fd_write && FD_ISSET(ioh->fd, &wfds)) {
> -                ioh->fd_write(ioh->opaque);
> -            }
> -
> -            /* Do this last in case read/write handlers marked it for deletion */
> -            if (ioh->deleted) {
> -                QLIST_REMOVE(ioh, next);
> -                qemu_free(ioh);
> -            }
> -        }
> -    }
>
> +    qemu_iohandler_poll(&rfds, &wfds, &xfds, ret);
>     slirp_select_poll(&rfds, &wfds, &xfds, (ret < 0));
>
>     qemu_run_all_timers();
> --
> 1.7.4
>
>
>
>

[Qemu-devel] [PATCH 2/2] add a service to reap zombies, use it in SLIRP

[Paolo Bonzini]

SLIRP -smb support wants to fork a process and forget about reaping it.
To please it, add a generic service to register a process id and let
QEMU reap it.  In the future it could be enhanced to pass a status,
but this would be unused.

With this in place, the SIGCHLD signal handler would not stomp on pclose
anymore.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 iohandler.c   |   64 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 os-posix.c    |    9 --------
 qemu-common.h |    1 +
 slirp/misc.c  |    5 +++-
 4 files changed, 69 insertions(+), 10 deletions(-)

diff --git a/iohandler.c b/iohandler.c
index 2e30fe3..2b82421 100644
--- a/iohandler.c
+++ b/iohandler.c
@@ -27,6 +27,10 @@
 #include "qemu-char.h"
 #include "qemu-queue.h"
 
+#ifndef _WIN32
+#include <sys/wait.h>
+#endif
+
 typedef struct IOHandlerRecord {
     int fd;
     IOCanReadHandler *fd_read_poll;
@@ -127,3 +131,63 @@ void qemu_iohandler_poll(fd_set *readfds, fd_set *writefds, fd_set *xfds, int re
         }
     }
 }
+
+/* reaping of zombies.  right now we're not passing the status to
+   anyone, but it would be possible to add a callback.  */
+#ifndef _WIN32
+typedef struct ChildProcessRecord {
+    int pid;
+    QLIST_ENTRY(ChildProcessRecord) next;
+} ChildProcessRecord;
+
+static QLIST_HEAD(, ChildProcessRecord) child_watches =
+    QLIST_HEAD_INITIALIZER(child_watches);
+
+static QEMUBH *sigchld_bh;
+
+static void sigchld_handler(int signal)
+{
+    qemu_bh_schedule(sigchld_bh);
+}
+
+static void sigchld_bh_handler(void *opaque)
+{
+    ChildProcessRecord *rec, *next;
+
+    QLIST_FOREACH_SAFE(rec, &child_watches, next, next) {
+        if (waitpid(rec->pid, NULL, WNOHANG) == rec->pid) {
+            QLIST_REMOVE(rec, next);
+            qemu_free(rec);
+        }
+    }
+}
+
+static void qemu_init_child_watch(void)
+{
+    struct sigaction act;
+    sigchld_bh = qemu_bh_new(sigchld_bh_handler, NULL);
+
+    act.sa_handler = sigchld_handler;
+    act.sa_flags = SA_NOCLDSTOP;
+    sigaction(SIGCHLD, &act, NULL);
+}
+
+int qemu_add_child_watch(pid_t pid)
+{
+    ChildProcessRecord *rec;
+
+    if (!sigchld_bh) {
+        qemu_init_child_watch();
+    }
+
+    QLIST_FOREACH(rec, &child_watches, next) {
+        if (rec->pid == pid) {
+            return 1;
+        }
+    }
+    rec = qemu_mallocz(sizeof(ChildProcessRecord));
+    rec->pid = pid;
+    QLIST_INSERT_HEAD(&child_watches, rec, next);
+    return 0;
+}
+#endif
diff --git a/os-posix.c b/os-posix.c
index 38c29d1..d9c17d8 100644
--- a/os-posix.c
+++ b/os-posix.c
@@ -66,11 +66,6 @@ static void termsig_handler(int signal)
     qemu_system_shutdown_request();
 }
 
-static void sigchld_handler(int signal)
-{
-    waitpid(-1, NULL, WNOHANG);
-}
-
 void os_setup_signal_handling(void)
 {
     struct sigaction act;
@@ -80,10 +75,6 @@ void os_setup_signal_handling(void)
     sigaction(SIGINT,  &act, NULL);
     sigaction(SIGHUP,  &act, NULL);
     sigaction(SIGTERM, &act, NULL);
-
-    act.sa_handler = sigchld_handler;
-    act.sa_flags = SA_NOCLDSTOP;
-    sigaction(SIGCHLD, &act, NULL);
 }
 
 /* Find a likely location for support files using the location of the binary.
diff --git a/qemu-common.h b/qemu-common.h
index 27855b0..c670c0e 100644
--- a/qemu-common.h
+++ b/qemu-common.h
@@ -210,6 +210,7 @@ ssize_t qemu_write_full(int fd, const void *buf, size_t count)
 void qemu_set_cloexec(int fd);
 
 #ifndef _WIN32
+int qemu_add_child_watch(pid_t pid);
 int qemu_eventfd(int pipefd[2]);
 int qemu_pipe(int pipefd[2]);
 #endif
diff --git a/slirp/misc.c b/slirp/misc.c
index 19dbec4..08eba6a 100644
--- a/slirp/misc.c
+++ b/slirp/misc.c
@@ -119,6 +119,7 @@ fork_exec(struct socket *so, const char *ex, int do_pty)
 	char *bptr;
 	const char *curarg;
 	int c, i, ret;
+	pid_t pid;
 
 	DEBUG_CALL("fork_exec");
 	DEBUG_ARG("so = %lx", (long)so);
@@ -142,7 +143,8 @@ fork_exec(struct socket *so, const char *ex, int do_pty)
 		}
 	}
 
-	switch(fork()) {
+	pid = fork();
+	switch(pid) {
 	 case -1:
 		lprint("Error: fork failed: %s\n", strerror(errno));
 		close(s);
@@ -206,6 +208,7 @@ fork_exec(struct socket *so, const char *ex, int do_pty)
 		exit(1);
 
 	 default:
+		qemu_add_child_watch(pid);
 		if (do_pty == 2) {
 			close(s);
 			so->s = master;
-- 
1.7.4

[Qemu-devel] Re: [PATCH 0/2] avoid races on exec migration

[Paolo Bonzini]

On 03/09/2011 06:21 PM, Paolo Bonzini wrote:
> QEMU has a sigchld handler that reaps any child process.  -smb is the
> only user of it and, in fact, QEMU inherited it from slirp.  However,
> this handler causes 'exec' based migration to randomly return 'status:
> failed' in the monitor.  This happens when the signal handler for SIGCHLD
> is ran before the pclose() of exec migration.
>
> The return status of fclose() is passed back as return status of
> qemu_fclose().  If qemu_fclose() fails, then the exec_close() in
> migration-exec.c returns a error code. This causes migrate_fd_cleanup()
> to return an error, and thus finally we see why 'status: failed' occurs:
>
>          if (migrate_fd_cleanup(s)<  0) {
>              if (old_vm_running) {
>                  vm_start();
>              }
>              state = MIG_STATE_ERROR;
>          }
>
> To avoid this, register the pids in a list and, on SIGCHLD, set up a
> bottom-half that would go through the pids and reap them.
>
> Since I'm at it, I'm moving iohandler stuff out of vl.c.  The new
> file isn't a perfect place to add the child watcher, but it's arguably
> better than vl.c.
>
> This should be applied to both master and stable.
>
> Paolo Bonzini (2):
>    extract I/O handler lists to iohandler.c
>    add a service to reap zombies
>
>   Makefile.objs |    2 +-
>   iohandler.c   |  193 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   os-posix.c    |    9 ---
>   qemu-common.h |    4 +
>   slirp/misc.c  |    5 +-
>   vl.c          |  106 ++------------------------------
>   6 files changed, 207 insertions(+), 112 deletions(-)
>   create mode 100644 iohandler.c
>

Ping?

Paolo

[Qemu-devel] Re: [PATCH 0/2] avoid races on exec migration

[Paolo Bonzini]

On 03/21/2011 09:24 AM, Paolo Bonzini wrote:
> On 03/09/2011 06:21 PM, Paolo Bonzini wrote:
>> QEMU has a sigchld handler that reaps any child process. -smb is the
>> only user of it and, in fact, QEMU inherited it from slirp. However,
>> this handler causes 'exec' based migration to randomly return 'status:
>> failed' in the monitor. This happens when the signal handler for SIGCHLD
>> is ran before the pclose() of exec migration.
>>
>> The return status of fclose() is passed back as return status of
>> qemu_fclose(). If qemu_fclose() fails, then the exec_close() in
>> migration-exec.c returns a error code. This causes migrate_fd_cleanup()
>> to return an error, and thus finally we see why 'status: failed' occurs:
>>
>> if (migrate_fd_cleanup(s)< 0) {
>> if (old_vm_running) {
>> vm_start();
>> }
>> state = MIG_STATE_ERROR;
>> }
>>
>> To avoid this, register the pids in a list and, on SIGCHLD, set up a
>> bottom-half that would go through the pids and reap them.
>>
>> Since I'm at it, I'm moving iohandler stuff out of vl.c. The new
>> file isn't a perfect place to add the child watcher, but it's arguably
>> better than vl.c.
>>
>> This should be applied to both master and stable.
>>
>> Paolo Bonzini (2):
>> extract I/O handler lists to iohandler.c
>> add a service to reap zombies
>>
>> Makefile.objs | 2 +-
>> iohandler.c | 193
>> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> os-posix.c | 9 ---
>> qemu-common.h | 4 +
>> slirp/misc.c | 5 +-
>> vl.c | 106 ++------------------------------
>> 6 files changed, 207 insertions(+), 112 deletions(-)
>> create mode 100644 iohandler.c
>>
>
> Ping?

Ping^2?

Paolo

Re: [Qemu-devel] [PATCH 0/2] avoid races on exec migration

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> QEMU has a sigchld handler that reaps any child process.  -smb is the
> only user of it and, in fact, QEMU inherited it from slirp.  However,
> this handler causes 'exec' based migration to randomly return 'status:
> failed' in the monitor.  This happens when the signal handler for SIGCHLD
> is ran before the pclose() of exec migration.

Signal handler uses undirected waitpid(), which can steal the zombie
from pclose() (race condition).  pclose() doesn't expect that, and
fails.

> The return status of fclose() is passed back as return status of
> qemu_fclose().  If qemu_fclose() fails, then the exec_close() in
> migration-exec.c returns a error code. This causes migrate_fd_cleanup()
> to return an error, and thus finally we see why 'status: failed' occurs:
>
>         if (migrate_fd_cleanup(s) < 0) {
>             if (old_vm_running) {
>                 vm_start();
>             }
>             state = MIG_STATE_ERROR;
>         }
>
> To avoid this, register the pids in a list and, on SIGCHLD, set up a
> bottom-half that would go through the pids and reap them.

Signal handler now waitpid()s only for registered children, so it can't
steal zombies anymore.

> Since I'm at it, I'm moving iohandler stuff out of vl.c.  The new
> file isn't a perfect place to add the child watcher, but it's arguably
> better than vl.c.

Pretty much anything's better than vl.c.

You silently drop unused IOHandlerRecord member ufd.  Dropping junk
good, silence not so good.

Not sure iohandler.c is the best home for qemu_add_child_watch() &
friends, but at least it's not vl.c ;)

> This should be applied to both master and stable.

Acked-by: Markus Armbruster <armbru@redhat.com>

[Qemu-devel] Re: [PATCH 0/2] avoid races on exec migration

[Paolo Bonzini]

On 03/29/2011 01:52 PM, Markus Armbruster wrote:
>> To avoid this, register the pids in a list and, on SIGCHLD, set up a
>> bottom-half that would go through the pids and reap them.
>
> Signal handler now waitpid()s only for registered children, so it can't
> steal zombies anymore.

Exactly.

>> Since I'm at it, I'm moving iohandler stuff out of vl.c.  The new
>> file isn't a perfect place to add the child watcher, but it's arguably
>> better than vl.c.
>
> Pretty much anything's better than vl.c.
>
> You silently drop unused IOHandlerRecord member ufd.  Dropping junk
> good, silence not so good.

1/2 had a pretty terse commit message overall.

Paolo

Re: [Qemu-devel] [PATCH 0/2] avoid races on exec migration

[Anthony Liguori]

On 03/09/2011 11:21 AM, Paolo Bonzini wrote:
> QEMU has a sigchld handler that reaps any child process.  -smb is the
> only user of it and, in fact, QEMU inherited it from slirp.  However,
> this handler causes 'exec' based migration to randomly return 'status:
> failed' in the monitor.  This happens when the signal handler for SIGCHLD
> is ran before the pclose() of exec migration.
>
> The return status of fclose() is passed back as return status of
> qemu_fclose().  If qemu_fclose() fails, then the exec_close() in
> migration-exec.c returns a error code. This causes migrate_fd_cleanup()
> to return an error, and thus finally we see why 'status: failed' occurs:
>
>          if (migrate_fd_cleanup(s)<  0) {
>              if (old_vm_running) {
>                  vm_start();
>              }
>              state = MIG_STATE_ERROR;
>          }
>
> To avoid this, register the pids in a list and, on SIGCHLD, set up a
> bottom-half that would go through the pids and reap them.
>
> Since I'm at it, I'm moving iohandler stuff out of vl.c.  The new
> file isn't a perfect place to add the child watcher, but it's arguably
> better than vl.c.
>
> This should be applied to both master and stable.

Applied all.  Thanks.

Regards,

Anthony Liguori

> Paolo Bonzini (2):
>    extract I/O handler lists to iohandler.c
>    add a service to reap zombies
>
>   Makefile.objs |    2 +-
>   iohandler.c   |  193 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   os-posix.c    |    9 ---
>   qemu-common.h |    4 +
>   slirp/misc.c  |    5 +-
>   vl.c          |  106 ++------------------------------
>   6 files changed, 207 insertions(+), 112 deletions(-)
>   create mode 100644 iohandler.c
>