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

["Daniel P. Berrangé" <berrange@redhat.com>]

On Mon, Oct 26, 2026 at 02:54:32PM +0100, Markus Armbruster wrote:
> 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?

Opps, yes, you are right - we should in fact fall through to
the cleanup: label, as the intention is 'sock' is closed in
all, as the caller expects socket_accept() to close the sock
in all cases. That's a second bug in my original patch.

This is pretty unpleasant semantics though - the caller owns
the socket and should own closing it.

>            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.
> 

With regards,
Daniel
-- 
|: https://berrange.com       ~~        https://hachyderm.io/@berrange :|
|: https://libvirt.org          ~~          https://entangle-photo.org :|
|: https://pixelfed.art/berrange   ~~    https://fstop138.berrange.com :|