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

[Markus Armbruster]

Paolo Bonzini <address@hidden> 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 <address@hidden>