Re: [PATCH 2/2] test/passt_in_ns: Consistent sleep commands before starting socat client

[Stefano Brivio <sbrivio@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2855 bytes --]

On Sat, 17 Sep 2022 13:32:45 +1000
David Gibson <david(a)gibson.dropbear.id.au> wrote:

> On Sat, Sep 17, 2022 at 01:55:34AM +0200, Stefano Brivio wrote:
> > There are some 'sleep 1' commands between starting the socat server
> > and its corresponding client to avoid races due to the server not
> > being ready as we start sending data.
> > 
> > However, those don't cover all the cases where we might need them,
> > and in some cases the sleep command actually ended up being before
> > the server even starts.
> > 
> > This fixes occasional failures in TCP and UDP simple transfer tests,
> > that became apparent with the new command dispatch mechanism.
> > 
> > Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com>  
> 
> Heh, I have a very similar patch in my upcoming series.  I used sleep
> 0.1 though, to avoid taking so long, which seems to be sufficient in
> practice.

Just mind POSIX only specifies integers as argument for sleep(1), a
pattern I commonly use is:

	sleep 0.1 || sleep 1

> I did look for a proper way to wait for the socat server to be ready,
> but I couldn't find anything workable.

I guess we could enable logging and look for "starting accept loop",
from _xioopen_listen() in xio-listen.c.

Anyway, right now, I'm just trying to get the tests to complete with
all your pending series, because it's been a while. This doesn't look
too bad, we can try to improve it later.

> I thought I could retry on the
> client side, but that doesn't work (at least for host to guest
> connections) because passt is always listening on the forwarded ports,
> so the client won't see a connection refused from the actual server.
> I don't think there's any sane way to propagate connection refused in
> that way, although we could and probably should forward connection
> resets outwards.

They should be already, and (manual) retries actually worked for me,
but it looks like added complexity. Before even checking the connection
state, tcp_tap_handler() checks for the RST flag:

	if (th->rst) {
		conn_event(c, conn, CLOSED);
		return p->count;
	}

and this will abruptly close the originating socket, which implies a
RST, on Linux.

We don't send out ICMP or ICMPv6 messages, even if the guest
additionally replied with one, because we can't ("ping" sockets are
just for that -- echoes). For TCP, a simple RST is probably not as
descriptive, but that's all we can do.

> Btw, I've also been doing a bunch of work on the static checks - with
> some different options I've brought the runtime of make cppcheck from
> ~40 minutes down to ~40 seconds :).

Whoa, 40 minutes? For me it was never more than a couple of minutes,
see https://passt.top/passt/about/#continuous-integration,
"build/static_checkers".

I guess it depends on system headers... but in any case that's
currently taking way too long, also for myself. :)

-- 
Stefano