On Tue, 2018-05-01 at 12:06 +0100, Daniel P. Berrangé wrote:
> The current socket test is rather crazy in that it sets up a server
> listening for sockets and then runs a client connect call, relying on
> the fact that the kernel will accept this despite the application
> not having called accept() yet. It then closes the client socket and
> calls accept() on the server. On Linux accept() will always see that
> the client has gone and so skip the rest of the code. On FreeBSD,
> however, the accept sometimes succeeds, causing us to then go into
> code that attempts to read and write to the client which will fail
> aborting the test. The accept() never succeeds on FreeBSD guests
Double space.
> with a single CPU, but as you add more CPUs, accept() becomes more and
> more likely to succeed, giving a 100% failure rate for the test when
> using 8 CPUs.
>
> This completely rewrites the test so that it is avoids this designed in
> race condition. We simply spawn a background thread to act as the
> client, which will read a byte from the server and write it back again.
Same.
[...]
> +static int
> +testSocketAccept(const void *opaque)
> {
> virNetSocketPtr *lsock = NULL; /* Listen socket */
> size_t nlsock = 0, i;
> virNetSocketPtr ssock = NULL; /* Server socket */
> - virNetSocketPtr csock = NULL; /* Client socket */
> + virNetSocketPtr rsock = NULL; /* Server from poll */
This comment doesn't look right...
[...]
> + virThread th;
> + struct testClientData cdata = { 0 };
> + bool goodsock = false;
> + char a = 'a', b = '\0';
Please put each variable declaration on a separate line.
[...]
> + if (virNetSocketWrite(ssock, &a, 1) < 0 ||
> + virNetSocketRead(ssock, &b, 1) < 0)
> + goto join;
Curly braces are required around the body here.
> + if (a != b) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + "Bad data received '%x' != '%x'", a, b);
> + goto join;
> + }
I'd leave an empty line here.
Everything else looks sensible enough and I've verified that
with these changes the issue no longer reproduces on my test
environment, so with the nits fixed
Reviewed-by: Andrea Bolognani <abologna@xxxxxxxxxx>
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
