On Mon, Oct 26, 2026 at 02:54:32PM +0100, Markus Armbruster wrote: > Daniel P. Berrangé <[email protected]> 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 <[email protected]> > > Fixes: c12ad5378739 (qtest: fail fast when QEMU terminates early) > > Signed-off-by: Daniel P. Berrangé <[email protected]> > > --- > > 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 :|
