[PATCH v2 0/2] qtst: fixes and improvements to socket_accept()

[PATCH v2 0/2] qtst: fixes and improvements to socket_accept()

[Daniel P. Berrangé]

Changed in v3:

 * Fix missing close() of listener sockets
 * Fix missing error message on final timeout
 * Push close of listener sockets up to caller

Daniel P. Berrangé (2):
  qtest: multiple fixes to socket_accept()
  qtest: don't close listener socket in qtest_socket_accept

 tests/qtest/libqtest.c | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

-- 
2.56.0

[PATCH v2 1/2] qtest: multiple fixes to socket_accept()

[Daniel P. Berrangé]

If the very first iteration of the loop breaks early due to the
dead QEMU process, 'ret' is never initialized. Furthermore the
listener socket is not closed on success and there is no error
message printed if timing out after all iterations are complete.
These problems were introduced by the recent refactoring to check
QEMU is alive.

CID 1688281
Reported-by: Markus Armbruster <armbru@redhat.com>
Fixes: c12ad5378739 ("qtest: fail fast when QEMU terminates early")
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 tests/qtest/libqtest.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
index 9e7a2d43a18..50487739ef9 100644
--- a/tests/qtest/libqtest.c
+++ b/tests/qtest/libqtest.c
@@ -121,7 +121,7 @@ static int qtest_socket_accept(QTestState *s, int sock)
 {
     struct sockaddr_un addr;
     socklen_t addrlen;
-    int ret;
+    int ret = -1;
     size_t i;
 #ifndef _WIN32
     struct timeval timeout = { .tv_sec = 0,
@@ -147,14 +147,13 @@ static int qtest_socket_accept(QTestState *s, int sock)
 
         addrlen = sizeof(addr);
         ret = accept(sock, (struct sockaddr *)&addr, &addrlen);
-        if (ret == -1) {
-            if (errno == EINTR || errno == EAGAIN) {
-                continue;
-            } else {
-                fprintf(stderr, "%s failed: %s\n", __func__, strerror(errno));
-            }
+        if (ret == -1 && (errno == EINTR || errno == EAGAIN)) {
+            continue;
         }
-        return ret;
+        break;
+    }
+    if (ret == -1) {
+        fprintf(stderr, "%s failed: %s\n", __func__, strerror(errno));
     }
 
  cleanup:
-- 
2.56.0

[PATCH v2 2/2] qtest: don't close listener socket in qtest_socket_accept

[Daniel P. Berrangé]

The listener FD passed into qtest_socket_accept should be
considered owned by the caller, and thus is is inappropriate
and unusual to close it in the method.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 tests/qtest/libqtest.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
index 50487739ef9..0f701679745 100644
--- a/tests/qtest/libqtest.c
+++ b/tests/qtest/libqtest.c
@@ -134,7 +134,6 @@ static int qtest_socket_accept(QTestState *s, int sock)
                    (void *)&timeout, sizeof(timeout))) {
         fprintf(stderr, "%s failed to set SO_RCVTIMEO: %s\n",
                 __func__, strerror(errno));
-        close(sock);
         return -1;
     }
 
@@ -142,7 +141,7 @@ static int qtest_socket_accept(QTestState *s, int sock)
         if (!qtest_probe_child(s)) {
             fprintf(stderr,
                     "child process unexpectedly exited, skipping socket accept\n");
-            goto cleanup;
+            return -1;
         }
 
         addrlen = sizeof(addr);
@@ -156,9 +155,6 @@ static int qtest_socket_accept(QTestState *s, int sock)
         fprintf(stderr, "%s failed: %s\n", __func__, strerror(errno));
     }
 
- cleanup:
-    close(sock);
-
     return ret;
 }
 
@@ -566,6 +562,8 @@ void qtest_connect(QTestState *s)
     unlink(socket_path);
     unlink(qmp_socket_path);
     g_assert(s->fd >= 0 && s->qmp_fd >= 0);
+    close(s->sock);
+    close(s->qmpsock);
     s->sock = s->qmpsock = -1;
     /* ask endianness of the target */
     s->big_endian = qtest_query_target_endianness(s);
@@ -675,6 +673,7 @@ QTestState *qtest_init_with_serial(const char *extra_args, int *sock_fd)
 
     *sock_fd = qtest_socket_accept(qts, sock_fd_init);
 
+    close(sock_fd_init);
     unlink(sock_path);
     g_free(sock_path);
     rmdir(sock_dir);
-- 
2.56.0

Re: [PATCH v2 1/2] qtest: multiple fixes to socket_accept()

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> If the very first iteration of the loop breaks early due to the
> dead QEMU process, 'ret' is never initialized. Furthermore the
> listener socket is not closed on success and there is no error
> message printed if timing out after all iterations are complete.
> These problems were introduced by the recent refactoring to check
> QEMU is alive.
>
> CID 1688281
> Reported-by: Markus Armbruster <armbru@redhat.com>
> Fixes: c12ad5378739 ("qtest: fail fast when QEMU terminates early")
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  tests/qtest/libqtest.c | 15 +++++++--------
>  1 file changed, 7 insertions(+), 8 deletions(-)
>
> diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
> index 9e7a2d43a18..50487739ef9 100644
> --- a/tests/qtest/libqtest.c
> +++ b/tests/qtest/libqtest.c
> @@ -121,7 +121,7 @@ static int qtest_socket_accept(QTestState *s, int sock)
>  {
>      struct sockaddr_un addr;
>      socklen_t addrlen;
> -    int ret;
> +    int ret = -1;

You need this to make [*] work.

>      size_t i;
>  #ifndef _WIN32
>      struct timeval timeout = { .tv_sec = 0,
> @@ -147,14 +147,13 @@ static int qtest_socket_accept(QTestState *s, int sock)
       for (i = 0; i < ACCEPT_RETRIES ; i++) {
           if (!qtest_probe_child(s)) {
               fprintf(stderr,
                       "child process unexpectedly exited, skipping socket accept\n");

[*] @ret comes from the initialization above in the first iteration
only.  In later iterations, it comes from assignment [**] below.

               goto cleanup;
           }
>  
>          addrlen = sizeof(addr);
>          ret = accept(sock, (struct sockaddr *)&addr, &addrlen);

[**]

> -        if (ret == -1) {
> -            if (errno == EINTR || errno == EAGAIN) {
> -                continue;
> -            } else {
> -                fprintf(stderr, "%s failed: %s\n", __func__, strerror(errno));
> -            }
> +        if (ret == -1 && (errno == EINTR || errno == EAGAIN)) {
> +            continue;
>          }
> -        return ret;
> +        break;
> +    }
> +    if (ret == -1) {

@ret comes from [**], because the loop runs at least once
(ACCEPT_RETRIES is positive).

> +        fprintf(stderr, "%s failed: %s\n", __func__, strerror(errno));
>      }
>  
>   cleanup:

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

Re: [PATCH v2 2/2] qtest: don't close listener socket in qtest_socket_accept

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> The listener FD passed into qtest_socket_accept should be
> considered owned by the caller, and thus is is inappropriate
> and unusual to close it in the method.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  tests/qtest/libqtest.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
> index 50487739ef9..0f701679745 100644
> --- a/tests/qtest/libqtest.c
> +++ b/tests/qtest/libqtest.c
> @@ -134,7 +134,6 @@ static int qtest_socket_accept(QTestState *s, int sock)
>                     (void *)&timeout, sizeof(timeout))) {
>          fprintf(stderr, "%s failed to set SO_RCVTIMEO: %s\n",
>                  __func__, strerror(errno));
> -        close(sock);
>          return -1;
>      }
>  
> @@ -142,7 +141,7 @@ static int qtest_socket_accept(QTestState *s, int sock)
>          if (!qtest_probe_child(s)) {
>              fprintf(stderr,
>                      "child process unexpectedly exited, skipping socket accept\n");
> -            goto cleanup;
> +            return -1;

This removes the need for initializing @ret.  Observation, not a demand.

>          }
>  
>          addrlen = sizeof(addr);
> @@ -156,9 +155,6 @@ static int qtest_socket_accept(QTestState *s, int sock)
>          fprintf(stderr, "%s failed: %s\n", __func__, strerror(errno));
>      }
>  
> - cleanup:
> -    close(sock);
> -
>      return ret;
>  }
>  
> @@ -566,6 +562,8 @@ void qtest_connect(QTestState *s)
>      unlink(socket_path);
>      unlink(qmp_socket_path);
>      g_assert(s->fd >= 0 && s->qmp_fd >= 0);
> +    close(s->sock);
> +    close(s->qmpsock);
>      s->sock = s->qmpsock = -1;
>      /* ask endianness of the target */
>      s->big_endian = qtest_query_target_endianness(s);
> @@ -675,6 +673,7 @@ QTestState *qtest_init_with_serial(const char *extra_args, int *sock_fd)
>  
>      *sock_fd = qtest_socket_accept(qts, sock_fd_init);
>  
> +    close(sock_fd_init);
>      unlink(sock_path);
>      g_free(sock_path);
>      rmdir(sock_dir);

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