Re: [Qemu-devel] [PATCH for-1.4] libqtest: Wait for the right child PID after killing QEMU

[Eduardo Habkost]

On Tue, Jan 29, 2013 at 08:14:24PM -0600, Anthony Liguori wrote:
> Andreas Färber <address@hidden> writes:
> 
> > Am 28.01.2013 19:15, schrieb Eduardo Habkost:
> >> When running "make check" with gcov enabled, we get the following
> >> message:
> >> 
> >>    hw/tmp105.gcda:cannot open data file, assuming not executed
> >> 
> >> The problem happens because:
> >> 
> >>  * tmp105-test exits before QEMU exits, because waitpid() at
> >>    qtest_quit() fails;
> >>  * waitpid() fails because there's another process already
> >>    waiting for the QEMU process;
> >>  * The process that is already waiting for QEMU is the child created by
> >>    qtest_init() to run system();
> >>  * qtest_quit() is incorrectly waiting for the QEMU PID directly instead
> >>    of the child created by qtest_init().
> >> 
> >> This fixes the problem by sending SIGTERM to QEMU, but waiting for the
> >> child process created by qtest_init() (that exits immediately after QEMU
> >> exits).
> >> 
> >> Reported-by: Andreas Färber <address@hidden>
> >> Signed-off-by: Eduardo Habkost <address@hidden>
> >
> > Thanks. Still need to test this...
> >
> > Anthony, might this by any chance fix your sparc 100% CPU issue?
> 
> I don't see how.  How would this impact the QEMU process?

The only consequence of this bug that I can see, is that the *-test
process may exit before the QEMU process exits, and may leave a zombie
child.

It may affect QEMU in case it tries to do something after receiving
SIGTERM, such as sending data through the closed monitor or qtest
sockets without handling the write errors properly.


> 
> Regards,
> 
> Anthony Liguori
> 
> 
> >
> > Andreas
> >
> >> ---
> >>  tests/libqtest.c | 7 +++++--
> >>  1 file changed, 5 insertions(+), 2 deletions(-)
> >> 
> >> diff --git a/tests/libqtest.c b/tests/libqtest.c
> >> index 913fa05..762dec4 100644
> >> --- a/tests/libqtest.c
> >> +++ b/tests/libqtest.c
> >> @@ -39,7 +39,8 @@ struct QTestState
> >>      int qmp_fd;
> >>      bool irq_level[MAX_IRQ];
> >>      GString *rx;
> >> -    gchar *pid_file;
> >> +    gchar *pid_file; /* QEMU PID file */
> >> +    int child_pid;   /* Child process created to execute QEMU */
> >>      char *socket_path, *qmp_socket_path;
> >>  };
> >>  
> >> @@ -144,6 +145,7 @@ QTestState *qtest_init(const char *extra_args)
> >>  
> >>      s->rx = g_string_new("");
> >>      s->pid_file = pid_file;
> >> +    s->child_pid = pid;
> >>      for (i = 0; i < MAX_IRQ; i++) {
> >>          s->irq_level[i] = false;
> >>      }
> >> @@ -165,8 +167,9 @@ void qtest_quit(QTestState *s)
> >>  
> >>      pid_t pid = qtest_qemu_pid(s);
> >>      if (pid != -1) {
> >> +        /* kill QEMU, but wait for the child created by us to run 
> >> system() */
> >>          kill(pid, SIGTERM);
> >> -        waitpid(pid, &status, 0);
> >> +        waitpid(s->child_pid, &status, 0);
> >>      }
> >>  
> >>      unlink(s->pid_file);
> >> 
> >
> >
> > -- 
> > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> > GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

-- 
Eduardo