From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:53732)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <stefanha@gmail.com>) id 1WNLz2-0004cy-1p
	for qemu-devel@nongnu.org; Tue, 11 Mar 2014 08:40:44 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <stefanha@gmail.com>) id 1WNLys-0001w7-Qa
	for qemu-devel@nongnu.org; Tue, 11 Mar 2014 08:40:35 -0400
Received: from mail-ee0-x234.google.com ([2a00:1450:4013:c00::234]:55442)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <stefanha@gmail.com>) id 1WNLys-0001vr-IY
	for qemu-devel@nongnu.org; Tue, 11 Mar 2014 08:40:26 -0400
Received: by mail-ee0-f52.google.com with SMTP id e49so3688043eek.11
	for <qemu-devel@nongnu.org>; Tue, 11 Mar 2014 05:40:25 -0700 (PDT)
Date: Tue, 11 Mar 2014 13:40:22 +0100
From: Stefan Hajnoczi <stefanha@gmail.com>
Message-ID: <20140311124022.GA7761@stefanha-thinkpad.redhat.com>
References: <1394532550-21857-1-git-send-email-marcel.a@redhat.com>
	<1394532550-21857-2-git-send-email-marcel.a@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1394532550-21857-2-git-send-email-marcel.a@redhat.com>
Subject: Re: [Qemu-devel] [PATCH V2 1/2] tests/libqtest: Fix possible
 deadlock in qtest initialization
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Marcel Apfelbaum <marcel.a@redhat.com>
Cc: kwolf@redhat.com, stefanha@redhat.com, qemu-devel@nongnu.org, armbru@redhat.com, aliguori@amazon.com, afaerber@suse.de

On Tue, Mar 11, 2014 at 12:09:09PM +0200, Marcel Apfelbaum wrote:
> @@ -78,12 +79,16 @@ static int socket_accept(int sock)
>      struct sockaddr_un addr;
>      socklen_t addrlen;
>      int ret;
> +    struct timeval timeout = { .tv_sec = SOCKET_TIMEOUT,
> +                               .tv_usec = 0 };
> +
> +    setsockopt(sock, SOL_SOCKET, SO_RCVTIMEO, (void *)&timeout,
> +               sizeof(timeout));
>  
>      addrlen = sizeof(addr);
>      do {
>          ret = accept(sock, (struct sockaddr *)&addr, &addrlen);
>      } while (ret == -1 && errno == EINTR);
> -    g_assert_no_errno(ret);
>      close(sock);

Did you mean to leave SO_RCVTIMEO set after this function completes?

> @@ -91,7 +96,7 @@ static int socket_accept(int sock)
>  
>  static void kill_qemu(QTestState *s)
>  {
> -    if (s->qemu_pid != -1) {
> +    if (s && s->qemu_pid != -1) {
>          kill(s->qemu_pid, SIGTERM);
>          waitpid(s->qemu_pid, NULL, 0);
>      }

This is a bug in libqtest.c, please don't silence the crash.

kill_qemu() gets called from the SIGABRT signal handler but I forgot
that global_qtest isn't initialized yet while qtest_init() executes.

In other words, the cleanup is broken if we fail inside qtest_init().
Can you drop this hunk and I'll send a patch to fix the underlying
issue?

> @@ -153,6 +158,8 @@ QTestState *qtest_init(const char *extra_args)
>      g_free(socket_path);
>      g_free(qmp_socket_path);
>  
> +    g_assert(s->fd >= 0 && s->qmp_fd >= 0);
> +

We probably shouldn't socket_accept() s->qmp_fd if s->fd already failed.
Otherwise we'll wait another 5 seconds for the timeout to explire:

    s->fd = socket_accept(sock);
    if (s->fd >= 0) {
        s->qmp_fd = socket_accept(qmpsock);
    }