Re: [PATCH] qtest: ensure socket_accept() return value is initialized

[Markus Armbruster <armbru@redhat.com>]

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. Ensure it is set
> to -1 to represent an invalid FD number.
>
> 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 | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
> index 9e7a2d43a18..899727d6d35 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,

Hmm, we do the same thing in several different ways:

   static int qtest_socket_accept(QTestState *s, int sock)
   {
       struct sockaddr_un addr;
       socklen_t addrlen;
       int ret;
       size_t i;
   #ifndef _WIN32
       struct timeval timeout = { .tv_sec = 0,
                                  .tv_usec = ACCEPT_TIMEOUT_MS * 1000ul };
   #else
       DWORD timeout = ACCEPT_TIMEOUT_MS;
   #endif

       if (setsockopt(sock, SOL_SOCKET, SO_RCVTIMEO,
                      (void *)&timeout, sizeof(timeout))) {
           fprintf(stderr, "%s failed to set SO_RCVTIMEO: %s\n",
                   __func__, strerror(errno));
           close(sock);
           return -1;

(A) Free resources and return failure.

       }

       for (i = 0; i < ACCEPT_RETRIES ; i++) {
           if (!qtest_probe_child(s)) {
               fprintf(stderr,
                       "child process unexpectedly exited, skipping socket accept\n");
               goto cleanup;

(B) Jump to exit, where we free resources and return @ret.

           }

           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));
               }
           }

(C) Return @ret.

Hmm, should we close(sock) on failure here?

           return ret;
       }

    cleanup:
       close(sock);

       return ret;
   }

All three ways can work fine.  But using all three next to each other I
find somewhat grating.  Matter of taste.  I'm not demanding you change
this.