[PATCH 0/2] test: Fix two issues made apparent by new command dispatch

[PATCH 0/2] test: Fix two issues made apparent by new command dispatch

[Stefano Brivio]

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

This is based on pending series by David, but they're logically separated
fixes, so I'm sending them as a different series.

Patch 2/2 adds some 'sleep 1' commands that were previously missing, which
actually goes against the intention of speeding up those tests. However,
without those, a number of tests fail quite frequently due to races
between starting server and client, and I could never get a clean run
with the new command dispatch mechanism otherwise.

We can probably find a more elegant (and time-saving) mechanism to
make sure the server is ready (based e.g. on parsing its output), but
at the moment I'm just trying to make the test suite work again for
me in its entirety.

Stefano Brivio (2):
  test/perf: Check for /sbin/sysctl with which(1), not simply sysctl
  test/passt_in_ns: Consistent sleep commands before starting socat
    client

 test/passt_in_ns/tcp | 29 +++++++++++++++++++++++++----
 test/passt_in_ns/udp | 18 ++++++++++++++++--
 test/perf/passt_tcp  |  4 ++--
 test/perf/passt_udp  |  2 +-
 test/perf/pasta_tcp  |  2 +-
 5 files changed, 45 insertions(+), 10 deletions(-)

-- 
2.35.1

[PATCH 1/2] test/perf: Check for /sbin/sysctl with which(1), not simply sysctl

[Stefano Brivio]

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

Otherwise, we're depending on having /sbin in $PATH. For some reason
I didn't completely grasp, with the new command dispatch mechanism
that's not the case anymore, even if I have /sbin in $PATH in the
parent shell.

Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com>
---
 test/perf/passt_tcp | 4 ++--
 test/perf/passt_udp | 2 +-
 test/perf/pasta_tcp | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/test/perf/passt_tcp b/test/perf/passt_tcp
index 45095b6..f0cfa1b 100644
--- a/test/perf/passt_tcp
+++ b/test/perf/passt_tcp
@@ -11,8 +11,8 @@
 # Copyright (c) 2021 Red Hat GmbH
 # Author: Stefano Brivio <sbrivio(a)redhat.com>
 
-gtools	sysctl ip jq nproc seq sleep iperf3 tcp_rr tcp_crr # From neper
-nstools	sysctl ip jq nproc seq sleep iperf3 tcp_rr tcp_crr
+gtools	/sbin/sysctl ip jq nproc seq sleep iperf3 tcp_rr tcp_crr # From neper
+nstools	/sbin/sysctl ip jq nproc seq sleep iperf3 tcp_rr tcp_crr
 htools	bc head sed seq
 
 # In this setup, virtio_net TX queue sometimes hangs, still under investigation
diff --git a/test/perf/passt_udp b/test/perf/passt_udp
index d199523..80731d1 100644
--- a/test/perf/passt_udp
+++ b/test/perf/passt_udp
@@ -11,7 +11,7 @@
 # Copyright (c) 2021 Red Hat GmbH
 # Author: Stefano Brivio <sbrivio(a)redhat.com>
 
-gtools	sysctl ip jq nproc sleep iperf3 udp_rr # From neper
+gtools	/sbin/sysctl ip jq nproc sleep iperf3 udp_rr # From neper
 nstools	ip jq sleep iperf3 udp_rr
 htools	bc head sed
 
diff --git a/test/perf/pasta_tcp b/test/perf/pasta_tcp
index 4c19364..602ce52 100644
--- a/test/perf/pasta_tcp
+++ b/test/perf/pasta_tcp
@@ -12,7 +12,7 @@
 # Author: Stefano Brivio <sbrivio(a)redhat.com>
 
 htools	head ip seq bc sleep iperf3 tcp_rr tcp_crr jq sed
-nstools	sysctl nproc ip seq sleep iperf3 tcp_rr tcp_crr jq sed
+nstools	/sbin/sysctl nproc ip seq sleep iperf3 tcp_rr tcp_crr jq sed
 
 test	pasta: throughput and latency (local connections)
 
-- 
@@ -12,7 +12,7 @@
 # Author: Stefano Brivio <sbrivio(a)redhat.com>
 
 htools	head ip seq bc sleep iperf3 tcp_rr tcp_crr jq sed
-nstools	sysctl nproc ip seq sleep iperf3 tcp_rr tcp_crr jq sed
+nstools	/sbin/sysctl nproc ip seq sleep iperf3 tcp_rr tcp_crr jq sed
 
 test	pasta: throughput and latency (local connections)
 
-- 
2.35.1

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

[Stefano Brivio]

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

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>
---
 test/passt_in_ns/tcp | 29 +++++++++++++++++++++++++----
 test/passt_in_ns/udp | 18 ++++++++++++++++--
 2 files changed, 41 insertions(+), 6 deletions(-)

diff --git a/test/passt_in_ns/tcp b/test/passt_in_ns/tcp
index a2cb667..71fe1c1 100644
--- a/test/passt_in_ns/tcp
+++ b/test/passt_in_ns/tcp
@@ -18,8 +18,8 @@ nstools	socat ip jq md5sum cut
 test	TCP/IPv4: host to guest: big transfer
 set	TEMP_BIG __STATEDIR__/big.img
 guestb	socat -u TCP4-LISTEN:10001 OPEN:test_big.bin,create,trunc
-sleep	1
 host	dd if=/dev/urandom bs=1M count=10 of=__TEMP_BIG__
+sleep	1
 host	socat -u OPEN:__TEMP_BIG__ TCP4:127.0.0.1:10001
 guestw
 hout	MD5_BIG md5sum __TEMP_BIG__ | cut -d' ' -f1
@@ -29,6 +29,7 @@ check	[ "__GUEST_MD5_BIG__" = "__MD5_BIG__" ]
 test	TCP/IPv4: host to ns: big transfer
 set	TEMP_NS_BIG __STATEDIR__/big_ns.img
 nsb	socat -u TCP4-LISTEN:10002 OPEN:__TEMP_NS_BIG__,create,trunc
+sleep	1
 host	socat -u OPEN:__TEMP_BIG__ TCP4:127.0.0.1:10002
 nsw
 nsout	NS_MD5_BIG md5sum __TEMP_NS_BIG__ | cut -d' ' -f1
@@ -37,6 +38,7 @@ check	[ "__NS_MD5_BIG__" = "__MD5_BIG__" ]
 test	TCP/IPv4: guest to host: big transfer
 hostb	socat -u TCP4-LISTEN:10003 OPEN:__TEMP_BIG__,create,trunc
 gout	GW ip -j -4 route show|jq -rM '.[] | select(.dst == "default").gateway'
+sleep	1
 guest	socat -u OPEN:test_big.bin TCP4:__GW__:10003
 hostw
 hout	HOST_MD5_BIG md5sum __TEMP_BIG__ | cut -d' ' -f1
@@ -44,14 +46,15 @@ check	[ "__HOST_MD5_BIG__" = "__MD5_BIG__" ]
 
 test	TCP/IPv4: guest to ns: big transfer
 nsb	socat -u TCP4-LISTEN:10002 OPEN:__TEMP_BIG__,create,trunc
+sleep	1
 guest	socat -u OPEN:test_big.bin TCP4:__GW__:10002
 nsw
 hout	HOST_MD5_BIG md5sum __TEMP_BIG__ | cut -d' ' -f1
 check	[ "__HOST_MD5_BIG__" = "__MD5_BIG__" ]
 
 test	TCP/IPv4: ns to host (spliced): big transfer
-sleep	1
 hostb	socat -u TCP4-LISTEN:10003 OPEN:__TEMP_BIG__,create,trunc
+sleep	1
 ns	socat -u OPEN:__TEMP_NS_BIG__ TCP4:127.0.0.1:10003
 hostw
 hout	HOST_MD5_BIG md5sum __TEMP_BIG__ | cut -d' ' -f1
@@ -59,6 +62,7 @@ check	[ "__HOST_MD5_BIG__" = "__MD5_BIG__" ]
 
 test	TCP/IPv4: ns to host (via tap): big transfer
 hostb	socat -u TCP4-LISTEN:10003 OPEN:__TEMP_BIG__,create,trunc
+sleep	1
 ns	socat -u OPEN:__TEMP_NS_BIG__ TCP4:__GW__:10003
 hostw
 hout	HOST_MD5_BIG md5sum __TEMP_BIG__ | cut -d' ' -f1
@@ -76,6 +80,7 @@ test	TCP/IPv4: ns to guest (using namespace address): big transfer
 guestb	socat -u TCP4-LISTEN:10001 OPEN:test_big.bin,create,trunc
 nsout	IFNAME ip -j link show | jq -rM '.[] | select(.link_type == "ether").ifname'
 nsout	ADDR ip -j -4 addr show|jq -rM '.[] | select(.ifname == "__IFNAME__").addr_info[0].local'
+sleep	1
 ns	socat -u OPEN:__TEMP_NS_BIG__ TCP4:__ADDR__:10001
 guestw
 gout	GUEST_MD5_BIG md5sum test_big.bin | cut -d' ' -f1
@@ -84,8 +89,8 @@ check	[ "__GUEST_MD5_BIG__" = "__MD5_BIG__" ]
 test	TCP/IPv4: host to guest: small transfer
 set	TEMP_SMALL __STATEDIR__/small.img
 guestb	socat -u TCP4-LISTEN:10001 OPEN:test_small.bin,create,trunc
-sleep	1
 host	dd if=/dev/urandom bs=2k count=100 of=__TEMP_SMALL__
+sleep	1
 host	socat -u OPEN:__TEMP_SMALL__ TCP4:127.0.0.1:10001
 guestw
 hout	MD5_SMALL md5sum __TEMP_SMALL__ | cut -d' ' -f1
@@ -95,6 +100,7 @@ check	[ "__GUEST_MD5_SMALL__" = "__MD5_SMALL__" ]
 test	TCP/IPv4: host to ns: small transfer
 set	TEMP_NS_SMALL __STATEDIR__/small_ns.img
 nsb	socat -u TCP4-LISTEN:10002 OPEN:__TEMP_NS_SMALL__,create,trunc
+sleep	1
 host	socat -u OPEN:__TEMP_SMALL__ TCP4:127.0.0.1:10002
 nsw
 nsout	NS_MD5_SMALL md5sum __TEMP_NS_SMALL__ | cut -d' ' -f1
@@ -103,6 +109,7 @@ check	[ "__NS_MD5_SMALL__" = "__MD5_SMALL__" ]
 test	TCP/IPv4: guest to host: small transfer
 hostb	socat -u TCP4-LISTEN:10003 OPEN:__TEMP_SMALL__,create,trunc
 gout	GW ip -j -4 route show|jq -rM '.[] | select(.dst == "default").gateway'
+sleep	1
 guest	socat -u OPEN:test_small.bin TCP4:__GW__:10003
 hostw
 hout	HOST_MD5_SMALL md5sum __TEMP_SMALL__ | cut -d' ' -f1
@@ -110,14 +117,15 @@ check	[ "__HOST_MD5_SMALL__" = "__MD5_SMALL__" ]
 
 test	TCP/IPv4: guest to ns: small transfer
 nsb	socat -u TCP4-LISTEN:10002 OPEN:__TEMP_SMALL__,create,trunc
+sleep	1
 guest	socat -u OPEN:test_small.bin TCP4:__GW__:10002
 nsw
 hout	HOST_MD5_SMALL md5sum __TEMP_SMALL__ | cut -d' ' -f1
 check	[ "__HOST_MD5_SMALL__" = "__MD5_SMALL__" ]
 
 test	TCP/IPv4: ns to host (spliced): small transfer
-sleep	1
 hostb	socat -u TCP4-LISTEN:10003 OPEN:__TEMP_SMALL__,create,trunc
+sleep	1
 ns	socat -u OPEN:__TEMP_NS_SMALL__ TCP4:127.0.0.1:10003
 hostw
 hout	HOST_MD5_SMALL md5sum __TEMP_SMALL__ | cut -d' ' -f1
@@ -125,6 +133,7 @@ check	[ "__HOST_MD5_SMALL__" = "__MD5_SMALL__" ]
 
 test	TCP/IPv4: ns to host (via tap): small transfer
 hostb	socat -u TCP4-LISTEN:10003 OPEN:__TEMP_SMALL__,create,trunc
+sleep	1
 ns	socat -u OPEN:__TEMP_NS_SMALL__ TCP4:__GW__:10003
 hostw
 hout	HOST_MD5_SMALL md5sum __TEMP_SMALL__ | cut -d' ' -f1
@@ -156,6 +165,7 @@ check	[ "__GUEST_MD5_BIG__" = "__MD5_BIG__" ]
 
 test	TCP/IPv6: host to ns: big transfer
 nsb	socat -u TCP6-LISTEN:10002 OPEN:__TEMP_NS_BIG__,create,trunc
+sleep	1
 host	socat -u OPEN:__TEMP_BIG__ TCP6:[::1]:10002
 nsw
 nsout	NS_MD5_BIG md5sum __TEMP_NS_BIG__ | cut -d' ' -f1
@@ -165,6 +175,7 @@ test	TCP/IPv6: guest to host: big transfer
 hostb	socat -u TCP6-LISTEN:10003 OPEN:__TEMP_BIG__,create,trunc
 gout	GW6 ip -j -6 route show|jq -rM '.[] | select(.dst == "default").gateway'
 gout	IFNAME ip -j link show | jq -rM '.[] | select(.link_type == "ether").ifname'
+sleep	1
 guest	socat -u OPEN:test_big.bin TCP6:[__GW6__%__IFNAME__]:10003
 hostw
 hout	HOST_MD5_BIG md5sum __TEMP_BIG__ | cut -d' ' -f1
@@ -172,6 +183,7 @@ check	[ "__HOST_MD5_BIG__" = "__MD5_BIG__" ]
 
 test	TCP/IPv6: guest to ns: big transfer
 nsb	socat -u TCP6-LISTEN:10002 OPEN:__TEMP_BIG__,create,trunc
+sleep	1
 guest	socat -u OPEN:test_big.bin TCP6:[__GW6__%__IFNAME__]:10002
 nsw
 hout	HOST_MD5_BIG md5sum __TEMP_BIG__ | cut -d' ' -f1
@@ -179,6 +191,7 @@ check	[ "__HOST_MD5_BIG__" = "__MD5_BIG__" ]
 
 test	TCP/IPv6: ns to host (spliced): big transfer
 hostb	socat -u TCP6-LISTEN:10003 OPEN:__TEMP_BIG__,create,trunc
+sleep	1
 ns	socat -u OPEN:__TEMP_NS_BIG__ TCP6:[::1]:10003
 hostw
 hout	HOST_MD5_BIG md5sum __TEMP_BIG__ | cut -d' ' -f1
@@ -187,6 +200,7 @@ check	[ "__HOST_MD5_BIG__" = "__MD5_BIG__" ]
 test	TCP/IPv6: ns to host (via tap): big transfer
 hostb	socat -u TCP6-LISTEN:10003 OPEN:__TEMP_BIG__,create,trunc
 nsout	IFNAME ip -j link show | jq -rM '.[] | select(.link_type == "ether").ifname'
+sleep	1
 ns	socat -u OPEN:__TEMP_NS_BIG__ TCP6:[__GW6__%__IFNAME__]:10003
 hostw
 hout	HOST_MD5_BIG md5sum __TEMP_BIG__ | cut -d' ' -f1
@@ -203,6 +217,7 @@ check	[ "__GUEST_MD5_BIG__" = "__MD5_BIG__" ]
 test	TCP/IPv6: ns to guest (using namespace address): big transfer
 guestb	socat -u TCP6-LISTEN:10001 OPEN:test_big.bin,create,trunc
 nsout	ADDR6 ip -j -6 addr show|jq -rM '.[] | select(.ifname == "__IFNAME__").addr_info[0].local'
+sleep	1
 ns	socat -u OPEN:__TEMP_NS_BIG__ TCP6:[__ADDR6__]:10001
 guestw
 gout	GUEST_MD5_BIG md5sum test_big.bin | cut -d' ' -f1
@@ -218,6 +233,7 @@ check	[ "__GUEST_MD5_SMALL__" = "__MD5_SMALL__" ]
 
 test	TCP/IPv6: host to ns: small transfer
 nsb	socat -u TCP6-LISTEN:10002 OPEN:__TEMP_NS_SMALL__,create,trunc
+sleep	1
 host	socat -u OPEN:__TEMP_SMALL__ TCP6:[::1]:10002
 nsw
 nsout	NS_MD5_SMALL md5sum __TEMP_NS_SMALL__ | cut -d' ' -f1
@@ -227,6 +243,7 @@ test	TCP/IPv6: guest to host: small transfer
 hostb	socat -u TCP6-LISTEN:10003 OPEN:__TEMP_SMALL__,create,trunc
 gout	GW6 ip -j -6 route show|jq -rM '.[] | select(.dst == "default").gateway'
 gout	IFNAME ip -j link show | jq -rM '.[] | select(.link_type == "ether").ifname'
+sleep	1
 guest	socat -u OPEN:test_small.bin TCP6:[__GW6__%__IFNAME__]:10003
 hostw
 hout	HOST_MD5_SMALL md5sum __TEMP_SMALL__ | cut -d' ' -f1
@@ -234,6 +251,7 @@ check	[ "__HOST_MD5_SMALL__" = "__MD5_SMALL__" ]
 
 test	TCP/IPv6: guest to ns: small transfer
 nsb	socat -u TCP6-LISTEN:10002 OPEN:__TEMP_SMALL__
+sleep	1
 guest	socat -u OPEN:test_small.bin TCP6:[__GW6__%__IFNAME__]:10002
 nsw
 hout	HOST_MD5_SMALL md5sum __TEMP_SMALL__ | cut -d' ' -f1
@@ -241,6 +259,7 @@ check	[ "__HOST_MD5_SMALL__" = "__MD5_SMALL__" ]
 
 test	TCP/IPv6: ns to host (spliced): small transfer
 hostb	socat -u TCP6-LISTEN:10003 OPEN:__TEMP_SMALL__,create,trunc
+sleep	1
 ns	socat -u OPEN:__TEMP_NS_SMALL__ TCP6:[::1]:10003
 hostw
 hout	HOST_MD5_SMALL md5sum __TEMP_SMALL__ | cut -d' ' -f1
@@ -249,6 +268,7 @@ check	[ "__HOST_MD5_SMALL__" = "__MD5_SMALL__" ]
 test	TCP/IPv6: ns to host (via tap): small transfer
 hostb	socat -u TCP6-LISTEN:10003 OPEN:__TEMP_SMALL__,create,trunc
 nsout	IFNAME ip -j link show | jq -rM '.[] | select(.link_type == "ether").ifname'
+sleep	1
 ns	socat -u OPEN:__TEMP_NS_SMALL__ TCP6:[__GW6__%__IFNAME__]:10003
 hostw
 hout	HOST_MD5_SMALL md5sum __TEMP_SMALL__ | cut -d' ' -f1
@@ -264,6 +284,7 @@ check	[ "__GUEST_MD5_SMALL__" = "__MD5_SMALL__" ]
 
 test	TCP/IPv6: ns to guest (using namespace address): small transfer
 guestb	socat -u TCP6-LISTEN:10001 OPEN:test_small.bin,create,trunc
+sleep	1
 ns	socat -u OPEN:__TEMP_NS_SMALL__ TCP6:[__ADDR6__]:10001
 guestw
 gout	GUEST_MD5_SMALL md5sum test_small.bin | cut -d' ' -f1
diff --git a/test/passt_in_ns/udp b/test/passt_in_ns/udp
index 5f01cbf..b7e9cf3 100644
--- a/test/passt_in_ns/udp
+++ b/test/passt_in_ns/udp
@@ -18,8 +18,8 @@ htools	dd socat ip jq md5sum cut
 test	UDP/IPv4: host to guest
 set	TEMP __STATEDIR__/data
 guestb	socat -u UDP4-LISTEN:10001,null-eof OPEN:test.bin,create,trunc
-sleep	1
 host	dd if=/dev/urandom bs=1k count=5 > __TEMP__
+sleep	1
 host	socat -u OPEN:__TEMP__ UDP4:127.0.0.1:10001,shut-null
 guestw
 hout	MD5 md5sum __TEMP__ | cut -d' ' -f1
@@ -29,6 +29,7 @@ check	[ "__GUEST_MD5__" = "__MD5__" ]
 test	UDP/IPv4: host to ns
 set	TEMP_NS __STATEDIR__/data_ns
 nsb	socat -u UDP4-LISTEN:10002,null-eof OPEN:__TEMP_NS__,create,trunc
+sleep	1
 host	socat -u OPEN:__TEMP__ UDP4:127.0.0.1:10002,shut-null
 nsw
 nsout	NS_MD5 md5sum __TEMP_NS__ | cut -d' ' -f1
@@ -37,6 +38,7 @@ check	[ "__NS_MD5__" = "__MD5__" ]
 test	UDP/IPv4: guest to host
 hostb	socat -u UDP4-LISTEN:10003,null-eof OPEN:__TEMP__,create,trunc
 gout	GW ip -j -4 route show|jq -rM '.[] | select(.dst == "default").gateway'
+sleep	1
 guest	socat -u OPEN:test.bin UDP4:__GW__:10003,shut-null
 hostw
 hout	HOST_MD5 md5sum __TEMP__ | cut -d' ' -f1
@@ -44,14 +46,15 @@ check	[ "__HOST_MD5__" = "__MD5__" ]
 
 test	UDP/IPv4: guest to ns
 nsb	socat -u UDP4-LISTEN:10002,null-eof OPEN:__TEMP_NS__,create,trunc
+sleep	1
 guest	socat -u OPEN:test.bin UDP4:__GW__:10002,shut-null
 nsw
 hout	HOST_MD5 md5sum __TEMP__ | cut -d' ' -f1
 check	[ "__HOST_MD5__" = "__MD5__" ]
 
 test	UDP/IPv4: ns to host (recvmmsg/sendmmsg)
-sleep	1
 hostb	socat -u UDP4-LISTEN:10003,null-eof OPEN:__TEMP__,create,trunc
+sleep	1
 ns	socat -u OPEN:__TEMP_NS__ UDP4:127.0.0.1:10003,shut-null
 hostw
 hout	HOST_MD5 md5sum __TEMP__ | cut -d' ' -f1
@@ -59,6 +62,7 @@ check	[ "__HOST_MD5__" = "__MD5__" ]
 
 test	UDP/IPv4: ns to host (via tap)
 hostb	socat -u UDP4-LISTEN:10003,null-eof OPEN:__TEMP__,create,trunc
+sleep	1
 ns	socat -u OPEN:__TEMP_NS__ UDP4:__GW__:10003,shut-null
 hostw
 hout	HOST_MD5 md5sum __TEMP__ | cut -d' ' -f1
@@ -66,6 +70,7 @@ check	[ "__HOST_MD5__" = "__MD5__" ]
 
 test	UDP/IPv4: ns to guest (using loopback address)
 guestb	socat -u UDP4-LISTEN:10001,null-eof OPEN:test.bin,create,trunc
+sleep	1
 ns	socat -u OPEN:__TEMP_NS__ UDP4:127.0.0.1:10001,shut-null
 guestw
 gout	GUEST_MD5 md5sum test.bin | cut -d' ' -f1
@@ -75,6 +80,7 @@ test	UDP/IPv4: ns to guest (using namespace address)
 guestb	socat -u UDP4-LISTEN:10001,null-eof OPEN:test.bin,create,trunc
 nsout	IFNAME ip -j link show | jq -rM '.[] | select(.link_type == "ether").ifname'
 nsout	ADDR ip -j -4 addr show|jq -rM '.[] | select(.ifname == "__IFNAME__").addr_info[0].local'
+sleep	1
 ns	socat -u OPEN:__TEMP_NS__ UDP4:__ADDR__:10001,shut-null
 guestw
 gout	GUEST_MD5 md5sum test.bin | cut -d' ' -f1
@@ -82,6 +88,7 @@ check	[ "__GUEST_MD5__" = "__MD5__" ]
 
 test	UDP/IPv6: host to guest
 guestb	socat -u UDP6-LISTEN:10001,null-eof OPEN:test.bin,create,trunc
+sleep	1
 host	socat -u OPEN:__TEMP__ UDP6:[::1]:10001,shut-null
 guestw
 gout	GUEST_MD5 md5sum test.bin | cut -d' ' -f1
@@ -89,6 +96,7 @@ check	[ "__GUEST_MD5__" = "__MD5__" ]
 
 test	UDP/IPv6: host to ns
 nsb	socat -u UDP6-LISTEN:10002,null-eof OPEN:__TEMP_NS__,create,trunc
+sleep	1
 host	socat -u OPEN:__TEMP__ UDP6:[::1]:10002,shut-null
 nsw
 nsout	NS_MD5 md5sum __TEMP_NS__ | cut -d' ' -f1
@@ -98,6 +106,7 @@ test	UDP/IPv6: guest to host
 hostb	socat -u UDP6-LISTEN:10003,null-eof OPEN:__TEMP__,create,trunc
 gout	GW6 ip -j -6 route show|jq -rM '.[] | select(.dst == "default").gateway'
 gout	IFNAME ip -j link show | jq -rM '.[] | select(.link_type == "ether").ifname'
+sleep	1
 guest	socat -u OPEN:test.bin UDP6:[__GW6__%__IFNAME__]:10003,shut-null
 hostw
 hout	HOST_MD5 md5sum __TEMP__ | cut -d' ' -f1
@@ -105,6 +114,7 @@ check	[ "__HOST_MD5__" = "__MD5__" ]
 
 test	UDP/IPv6: guest to ns
 nsb	socat -u UDP6-LISTEN:10002,null-eof OPEN:__TEMP_NS__,create,trunc
+sleep	1
 guest	socat -u OPEN:test.bin UDP6:[__GW6__%__IFNAME__]:10002,shut-null
 nsw
 hout	HOST_MD5 md5sum __TEMP__ | cut -d' ' -f1
@@ -112,6 +122,7 @@ check	[ "__HOST_MD5__" = "__MD5__" ]
 
 test	UDP/IPv6: ns to host (recvmmsg/sendmmsg)
 hostb	socat -u UDP6-LISTEN:10003,null-eof OPEN:__TEMP__,create,trunc
+sleep	1
 ns	socat -u OPEN:__TEMP_NS__ UDP6:[::1]:10003,shut-null
 hostw
 hout	HOST_MD5 md5sum __TEMP__ | cut -d' ' -f1
@@ -120,6 +131,7 @@ check	[ "__HOST_MD5__" = "__MD5__" ]
 test	UDP/IPv6: ns to host (via tap)
 hostb	socat -u UDP6-LISTEN:10003,null-eof OPEN:__TEMP__,create,trunc
 nsout	IFNAME ip -j link show | jq -rM '.[] | select(.link_type == "ether").ifname'
+sleep	1
 ns	socat -u OPEN:__TEMP_NS__ UDP6:[__GW6__%__IFNAME__]:10003,shut-null
 hostw
 hout	HOST_MD5 md5sum __TEMP__ | cut -d' ' -f1
@@ -127,6 +139,7 @@ check	[ "__HOST_MD5__" = "__MD5__" ]
 
 test	UDP/IPv6: ns to guest (using loopback address)
 guestb	socat -u UDP6-LISTEN:10001,null-eof OPEN:test.bin,create,trunc
+sleep	1
 ns	socat -u OPEN:__TEMP_NS__ UDP6:[::1]:10001,shut-null
 guestw
 gout	GUEST_MD5 md5sum test.bin | cut -d' ' -f1
@@ -135,6 +148,7 @@ check	[ "__GUEST_MD5__" = "__MD5__" ]
 test	UDP/IPv6: ns to guest (using namespace address)
 guestb	socat -u UDP6-LISTEN:10001,null-eof OPEN:test.bin,create,trunc
 nsout	ADDR6 ip -j -6 addr show|jq -rM '.[] | select(.ifname == "__IFNAME__").addr_info[0].local'
+sleep	1
 ns	socat -u OPEN:__TEMP_NS__ UDP6:[__ADDR6__]:10001,shut-null
 guestw
 gout	GUEST_MD5 md5sum test.bin | cut -d' ' -f1
-- 
@@ -18,8 +18,8 @@ htools	dd socat ip jq md5sum cut
 test	UDP/IPv4: host to guest
 set	TEMP __STATEDIR__/data
 guestb	socat -u UDP4-LISTEN:10001,null-eof OPEN:test.bin,create,trunc
-sleep	1
 host	dd if=/dev/urandom bs=1k count=5 > __TEMP__
+sleep	1
 host	socat -u OPEN:__TEMP__ UDP4:127.0.0.1:10001,shut-null
 guestw
 hout	MD5 md5sum __TEMP__ | cut -d' ' -f1
@@ -29,6 +29,7 @@ check	[ "__GUEST_MD5__" = "__MD5__" ]
 test	UDP/IPv4: host to ns
 set	TEMP_NS __STATEDIR__/data_ns
 nsb	socat -u UDP4-LISTEN:10002,null-eof OPEN:__TEMP_NS__,create,trunc
+sleep	1
 host	socat -u OPEN:__TEMP__ UDP4:127.0.0.1:10002,shut-null
 nsw
 nsout	NS_MD5 md5sum __TEMP_NS__ | cut -d' ' -f1
@@ -37,6 +38,7 @@ check	[ "__NS_MD5__" = "__MD5__" ]
 test	UDP/IPv4: guest to host
 hostb	socat -u UDP4-LISTEN:10003,null-eof OPEN:__TEMP__,create,trunc
 gout	GW ip -j -4 route show|jq -rM '.[] | select(.dst == "default").gateway'
+sleep	1
 guest	socat -u OPEN:test.bin UDP4:__GW__:10003,shut-null
 hostw
 hout	HOST_MD5 md5sum __TEMP__ | cut -d' ' -f1
@@ -44,14 +46,15 @@ check	[ "__HOST_MD5__" = "__MD5__" ]
 
 test	UDP/IPv4: guest to ns
 nsb	socat -u UDP4-LISTEN:10002,null-eof OPEN:__TEMP_NS__,create,trunc
+sleep	1
 guest	socat -u OPEN:test.bin UDP4:__GW__:10002,shut-null
 nsw
 hout	HOST_MD5 md5sum __TEMP__ | cut -d' ' -f1
 check	[ "__HOST_MD5__" = "__MD5__" ]
 
 test	UDP/IPv4: ns to host (recvmmsg/sendmmsg)
-sleep	1
 hostb	socat -u UDP4-LISTEN:10003,null-eof OPEN:__TEMP__,create,trunc
+sleep	1
 ns	socat -u OPEN:__TEMP_NS__ UDP4:127.0.0.1:10003,shut-null
 hostw
 hout	HOST_MD5 md5sum __TEMP__ | cut -d' ' -f1
@@ -59,6 +62,7 @@ check	[ "__HOST_MD5__" = "__MD5__" ]
 
 test	UDP/IPv4: ns to host (via tap)
 hostb	socat -u UDP4-LISTEN:10003,null-eof OPEN:__TEMP__,create,trunc
+sleep	1
 ns	socat -u OPEN:__TEMP_NS__ UDP4:__GW__:10003,shut-null
 hostw
 hout	HOST_MD5 md5sum __TEMP__ | cut -d' ' -f1
@@ -66,6 +70,7 @@ check	[ "__HOST_MD5__" = "__MD5__" ]
 
 test	UDP/IPv4: ns to guest (using loopback address)
 guestb	socat -u UDP4-LISTEN:10001,null-eof OPEN:test.bin,create,trunc
+sleep	1
 ns	socat -u OPEN:__TEMP_NS__ UDP4:127.0.0.1:10001,shut-null
 guestw
 gout	GUEST_MD5 md5sum test.bin | cut -d' ' -f1
@@ -75,6 +80,7 @@ test	UDP/IPv4: ns to guest (using namespace address)
 guestb	socat -u UDP4-LISTEN:10001,null-eof OPEN:test.bin,create,trunc
 nsout	IFNAME ip -j link show | jq -rM '.[] | select(.link_type == "ether").ifname'
 nsout	ADDR ip -j -4 addr show|jq -rM '.[] | select(.ifname == "__IFNAME__").addr_info[0].local'
+sleep	1
 ns	socat -u OPEN:__TEMP_NS__ UDP4:__ADDR__:10001,shut-null
 guestw
 gout	GUEST_MD5 md5sum test.bin | cut -d' ' -f1
@@ -82,6 +88,7 @@ check	[ "__GUEST_MD5__" = "__MD5__" ]
 
 test	UDP/IPv6: host to guest
 guestb	socat -u UDP6-LISTEN:10001,null-eof OPEN:test.bin,create,trunc
+sleep	1
 host	socat -u OPEN:__TEMP__ UDP6:[::1]:10001,shut-null
 guestw
 gout	GUEST_MD5 md5sum test.bin | cut -d' ' -f1
@@ -89,6 +96,7 @@ check	[ "__GUEST_MD5__" = "__MD5__" ]
 
 test	UDP/IPv6: host to ns
 nsb	socat -u UDP6-LISTEN:10002,null-eof OPEN:__TEMP_NS__,create,trunc
+sleep	1
 host	socat -u OPEN:__TEMP__ UDP6:[::1]:10002,shut-null
 nsw
 nsout	NS_MD5 md5sum __TEMP_NS__ | cut -d' ' -f1
@@ -98,6 +106,7 @@ test	UDP/IPv6: guest to host
 hostb	socat -u UDP6-LISTEN:10003,null-eof OPEN:__TEMP__,create,trunc
 gout	GW6 ip -j -6 route show|jq -rM '.[] | select(.dst == "default").gateway'
 gout	IFNAME ip -j link show | jq -rM '.[] | select(.link_type == "ether").ifname'
+sleep	1
 guest	socat -u OPEN:test.bin UDP6:[__GW6__%__IFNAME__]:10003,shut-null
 hostw
 hout	HOST_MD5 md5sum __TEMP__ | cut -d' ' -f1
@@ -105,6 +114,7 @@ check	[ "__HOST_MD5__" = "__MD5__" ]
 
 test	UDP/IPv6: guest to ns
 nsb	socat -u UDP6-LISTEN:10002,null-eof OPEN:__TEMP_NS__,create,trunc
+sleep	1
 guest	socat -u OPEN:test.bin UDP6:[__GW6__%__IFNAME__]:10002,shut-null
 nsw
 hout	HOST_MD5 md5sum __TEMP__ | cut -d' ' -f1
@@ -112,6 +122,7 @@ check	[ "__HOST_MD5__" = "__MD5__" ]
 
 test	UDP/IPv6: ns to host (recvmmsg/sendmmsg)
 hostb	socat -u UDP6-LISTEN:10003,null-eof OPEN:__TEMP__,create,trunc
+sleep	1
 ns	socat -u OPEN:__TEMP_NS__ UDP6:[::1]:10003,shut-null
 hostw
 hout	HOST_MD5 md5sum __TEMP__ | cut -d' ' -f1
@@ -120,6 +131,7 @@ check	[ "__HOST_MD5__" = "__MD5__" ]
 test	UDP/IPv6: ns to host (via tap)
 hostb	socat -u UDP6-LISTEN:10003,null-eof OPEN:__TEMP__,create,trunc
 nsout	IFNAME ip -j link show | jq -rM '.[] | select(.link_type == "ether").ifname'
+sleep	1
 ns	socat -u OPEN:__TEMP_NS__ UDP6:[__GW6__%__IFNAME__]:10003,shut-null
 hostw
 hout	HOST_MD5 md5sum __TEMP__ | cut -d' ' -f1
@@ -127,6 +139,7 @@ check	[ "__HOST_MD5__" = "__MD5__" ]
 
 test	UDP/IPv6: ns to guest (using loopback address)
 guestb	socat -u UDP6-LISTEN:10001,null-eof OPEN:test.bin,create,trunc
+sleep	1
 ns	socat -u OPEN:__TEMP_NS__ UDP6:[::1]:10001,shut-null
 guestw
 gout	GUEST_MD5 md5sum test.bin | cut -d' ' -f1
@@ -135,6 +148,7 @@ check	[ "__GUEST_MD5__" = "__MD5__" ]
 test	UDP/IPv6: ns to guest (using namespace address)
 guestb	socat -u UDP6-LISTEN:10001,null-eof OPEN:test.bin,create,trunc
 nsout	ADDR6 ip -j -6 addr show|jq -rM '.[] | select(.ifname == "__IFNAME__").addr_info[0].local'
+sleep	1
 ns	socat -u OPEN:__TEMP_NS__ UDP6:[__ADDR6__]:10001,shut-null
 guestw
 gout	GUEST_MD5 md5sum test.bin | cut -d' ' -f1
-- 
2.35.1

Re: [PATCH 1/2] test/perf: Check for /sbin/sysctl with which(1), not simply sysctl

[David Gibson]

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

On Sat, Sep 17, 2022 at 01:55:33AM +0200, Stefano Brivio wrote:
> Otherwise, we're depending on having /sbin in $PATH. For some reason

Huh.  I wonder why this is working for me.

> I didn't completely grasp, with the new command dispatch mechanism
> that's not the case anymore, even if I have /sbin in $PATH in the
> parent shell.

For the guest, there's no mystery - there's no strong connection
between the shell environments there.  We probably can control PATH
with our mbuto setup though.

For the ns that is strange, though.

> 
> Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com>
> ---
>  test/perf/passt_tcp | 4 ++--
>  test/perf/passt_udp | 2 +-
>  test/perf/pasta_tcp | 2 +-
>  3 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/test/perf/passt_tcp b/test/perf/passt_tcp
> index 45095b6..f0cfa1b 100644
> --- a/test/perf/passt_tcp
> +++ b/test/perf/passt_tcp
> @@ -11,8 +11,8 @@
>  # Copyright (c) 2021 Red Hat GmbH
>  # Author: Stefano Brivio <sbrivio(a)redhat.com>
>  
> -gtools	sysctl ip jq nproc seq sleep iperf3 tcp_rr tcp_crr # From neper
> -nstools	sysctl ip jq nproc seq sleep iperf3 tcp_rr tcp_crr
> +gtools	/sbin/sysctl ip jq nproc seq sleep iperf3 tcp_rr tcp_crr # From neper
> +nstools	/sbin/sysctl ip jq nproc seq sleep iperf3 tcp_rr tcp_crr
>  htools	bc head sed seq
>  
>  # In this setup, virtio_net TX queue sometimes hangs, still under investigation
> diff --git a/test/perf/passt_udp b/test/perf/passt_udp
> index d199523..80731d1 100644
> --- a/test/perf/passt_udp
> +++ b/test/perf/passt_udp
> @@ -11,7 +11,7 @@
>  # Copyright (c) 2021 Red Hat GmbH
>  # Author: Stefano Brivio <sbrivio(a)redhat.com>
>  
> -gtools	sysctl ip jq nproc sleep iperf3 udp_rr # From neper
> +gtools	/sbin/sysctl ip jq nproc sleep iperf3 udp_rr # From neper
>  nstools	ip jq sleep iperf3 udp_rr
>  htools	bc head sed
>  
> diff --git a/test/perf/pasta_tcp b/test/perf/pasta_tcp
> index 4c19364..602ce52 100644
> --- a/test/perf/pasta_tcp
> +++ b/test/perf/pasta_tcp
> @@ -12,7 +12,7 @@
>  # Author: Stefano Brivio <sbrivio(a)redhat.com>
>  
>  htools	head ip seq bc sleep iperf3 tcp_rr tcp_crr jq sed
> -nstools	sysctl nproc ip seq sleep iperf3 tcp_rr tcp_crr jq sed
> +nstools	/sbin/sysctl nproc ip seq sleep iperf3 tcp_rr tcp_crr jq sed
>  
>  test	pasta: throughput and latency (local connections)
>  

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[David Gibson]

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

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.

I did look for a proper way to wait for the socat server to be ready,
but I couldn't find anything workable.  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.

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

> ---
>  test/passt_in_ns/tcp | 29 +++++++++++++++++++++++++----
>  test/passt_in_ns/udp | 18 ++++++++++++++++--
>  2 files changed, 41 insertions(+), 6 deletions(-)
> 
> diff --git a/test/passt_in_ns/tcp b/test/passt_in_ns/tcp
> index a2cb667..71fe1c1 100644
> --- a/test/passt_in_ns/tcp
> +++ b/test/passt_in_ns/tcp
> @@ -18,8 +18,8 @@ nstools	socat ip jq md5sum cut
>  test	TCP/IPv4: host to guest: big transfer
>  set	TEMP_BIG __STATEDIR__/big.img
>  guestb	socat -u TCP4-LISTEN:10001 OPEN:test_big.bin,create,trunc
> -sleep	1
>  host	dd if=/dev/urandom bs=1M count=10 of=__TEMP_BIG__
> +sleep	1
>  host	socat -u OPEN:__TEMP_BIG__ TCP4:127.0.0.1:10001
>  guestw
>  hout	MD5_BIG md5sum __TEMP_BIG__ | cut -d' ' -f1
> @@ -29,6 +29,7 @@ check	[ "__GUEST_MD5_BIG__" = "__MD5_BIG__" ]
>  test	TCP/IPv4: host to ns: big transfer
>  set	TEMP_NS_BIG __STATEDIR__/big_ns.img
>  nsb	socat -u TCP4-LISTEN:10002 OPEN:__TEMP_NS_BIG__,create,trunc
> +sleep	1
>  host	socat -u OPEN:__TEMP_BIG__ TCP4:127.0.0.1:10002
>  nsw
>  nsout	NS_MD5_BIG md5sum __TEMP_NS_BIG__ | cut -d' ' -f1
> @@ -37,6 +38,7 @@ check	[ "__NS_MD5_BIG__" = "__MD5_BIG__" ]
>  test	TCP/IPv4: guest to host: big transfer
>  hostb	socat -u TCP4-LISTEN:10003 OPEN:__TEMP_BIG__,create,trunc
>  gout	GW ip -j -4 route show|jq -rM '.[] | select(.dst == "default").gateway'
> +sleep	1
>  guest	socat -u OPEN:test_big.bin TCP4:__GW__:10003
>  hostw
>  hout	HOST_MD5_BIG md5sum __TEMP_BIG__ | cut -d' ' -f1
> @@ -44,14 +46,15 @@ check	[ "__HOST_MD5_BIG__" = "__MD5_BIG__" ]
>  
>  test	TCP/IPv4: guest to ns: big transfer
>  nsb	socat -u TCP4-LISTEN:10002 OPEN:__TEMP_BIG__,create,trunc
> +sleep	1
>  guest	socat -u OPEN:test_big.bin TCP4:__GW__:10002
>  nsw
>  hout	HOST_MD5_BIG md5sum __TEMP_BIG__ | cut -d' ' -f1
>  check	[ "__HOST_MD5_BIG__" = "__MD5_BIG__" ]
>  
>  test	TCP/IPv4: ns to host (spliced): big transfer
> -sleep	1
>  hostb	socat -u TCP4-LISTEN:10003 OPEN:__TEMP_BIG__,create,trunc
> +sleep	1
>  ns	socat -u OPEN:__TEMP_NS_BIG__ TCP4:127.0.0.1:10003
>  hostw
>  hout	HOST_MD5_BIG md5sum __TEMP_BIG__ | cut -d' ' -f1
> @@ -59,6 +62,7 @@ check	[ "__HOST_MD5_BIG__" = "__MD5_BIG__" ]
>  
>  test	TCP/IPv4: ns to host (via tap): big transfer
>  hostb	socat -u TCP4-LISTEN:10003 OPEN:__TEMP_BIG__,create,trunc
> +sleep	1
>  ns	socat -u OPEN:__TEMP_NS_BIG__ TCP4:__GW__:10003
>  hostw
>  hout	HOST_MD5_BIG md5sum __TEMP_BIG__ | cut -d' ' -f1
> @@ -76,6 +80,7 @@ test	TCP/IPv4: ns to guest (using namespace address): big transfer
>  guestb	socat -u TCP4-LISTEN:10001 OPEN:test_big.bin,create,trunc
>  nsout	IFNAME ip -j link show | jq -rM '.[] | select(.link_type == "ether").ifname'
>  nsout	ADDR ip -j -4 addr show|jq -rM '.[] | select(.ifname == "__IFNAME__").addr_info[0].local'
> +sleep	1
>  ns	socat -u OPEN:__TEMP_NS_BIG__ TCP4:__ADDR__:10001
>  guestw
>  gout	GUEST_MD5_BIG md5sum test_big.bin | cut -d' ' -f1
> @@ -84,8 +89,8 @@ check	[ "__GUEST_MD5_BIG__" = "__MD5_BIG__" ]
>  test	TCP/IPv4: host to guest: small transfer
>  set	TEMP_SMALL __STATEDIR__/small.img
>  guestb	socat -u TCP4-LISTEN:10001 OPEN:test_small.bin,create,trunc
> -sleep	1
>  host	dd if=/dev/urandom bs=2k count=100 of=__TEMP_SMALL__
> +sleep	1
>  host	socat -u OPEN:__TEMP_SMALL__ TCP4:127.0.0.1:10001
>  guestw
>  hout	MD5_SMALL md5sum __TEMP_SMALL__ | cut -d' ' -f1
> @@ -95,6 +100,7 @@ check	[ "__GUEST_MD5_SMALL__" = "__MD5_SMALL__" ]
>  test	TCP/IPv4: host to ns: small transfer
>  set	TEMP_NS_SMALL __STATEDIR__/small_ns.img
>  nsb	socat -u TCP4-LISTEN:10002 OPEN:__TEMP_NS_SMALL__,create,trunc
> +sleep	1
>  host	socat -u OPEN:__TEMP_SMALL__ TCP4:127.0.0.1:10002
>  nsw
>  nsout	NS_MD5_SMALL md5sum __TEMP_NS_SMALL__ | cut -d' ' -f1
> @@ -103,6 +109,7 @@ check	[ "__NS_MD5_SMALL__" = "__MD5_SMALL__" ]
>  test	TCP/IPv4: guest to host: small transfer
>  hostb	socat -u TCP4-LISTEN:10003 OPEN:__TEMP_SMALL__,create,trunc
>  gout	GW ip -j -4 route show|jq -rM '.[] | select(.dst == "default").gateway'
> +sleep	1
>  guest	socat -u OPEN:test_small.bin TCP4:__GW__:10003
>  hostw
>  hout	HOST_MD5_SMALL md5sum __TEMP_SMALL__ | cut -d' ' -f1
> @@ -110,14 +117,15 @@ check	[ "__HOST_MD5_SMALL__" = "__MD5_SMALL__" ]
>  
>  test	TCP/IPv4: guest to ns: small transfer
>  nsb	socat -u TCP4-LISTEN:10002 OPEN:__TEMP_SMALL__,create,trunc
> +sleep	1
>  guest	socat -u OPEN:test_small.bin TCP4:__GW__:10002
>  nsw
>  hout	HOST_MD5_SMALL md5sum __TEMP_SMALL__ | cut -d' ' -f1
>  check	[ "__HOST_MD5_SMALL__" = "__MD5_SMALL__" ]
>  
>  test	TCP/IPv4: ns to host (spliced): small transfer
> -sleep	1
>  hostb	socat -u TCP4-LISTEN:10003 OPEN:__TEMP_SMALL__,create,trunc
> +sleep	1
>  ns	socat -u OPEN:__TEMP_NS_SMALL__ TCP4:127.0.0.1:10003
>  hostw
>  hout	HOST_MD5_SMALL md5sum __TEMP_SMALL__ | cut -d' ' -f1
> @@ -125,6 +133,7 @@ check	[ "__HOST_MD5_SMALL__" = "__MD5_SMALL__" ]
>  
>  test	TCP/IPv4: ns to host (via tap): small transfer
>  hostb	socat -u TCP4-LISTEN:10003 OPEN:__TEMP_SMALL__,create,trunc
> +sleep	1
>  ns	socat -u OPEN:__TEMP_NS_SMALL__ TCP4:__GW__:10003
>  hostw
>  hout	HOST_MD5_SMALL md5sum __TEMP_SMALL__ | cut -d' ' -f1
> @@ -156,6 +165,7 @@ check	[ "__GUEST_MD5_BIG__" = "__MD5_BIG__" ]
>  
>  test	TCP/IPv6: host to ns: big transfer
>  nsb	socat -u TCP6-LISTEN:10002 OPEN:__TEMP_NS_BIG__,create,trunc
> +sleep	1
>  host	socat -u OPEN:__TEMP_BIG__ TCP6:[::1]:10002
>  nsw
>  nsout	NS_MD5_BIG md5sum __TEMP_NS_BIG__ | cut -d' ' -f1
> @@ -165,6 +175,7 @@ test	TCP/IPv6: guest to host: big transfer
>  hostb	socat -u TCP6-LISTEN:10003 OPEN:__TEMP_BIG__,create,trunc
>  gout	GW6 ip -j -6 route show|jq -rM '.[] | select(.dst == "default").gateway'
>  gout	IFNAME ip -j link show | jq -rM '.[] | select(.link_type == "ether").ifname'
> +sleep	1
>  guest	socat -u OPEN:test_big.bin TCP6:[__GW6__%__IFNAME__]:10003
>  hostw
>  hout	HOST_MD5_BIG md5sum __TEMP_BIG__ | cut -d' ' -f1
> @@ -172,6 +183,7 @@ check	[ "__HOST_MD5_BIG__" = "__MD5_BIG__" ]
>  
>  test	TCP/IPv6: guest to ns: big transfer
>  nsb	socat -u TCP6-LISTEN:10002 OPEN:__TEMP_BIG__,create,trunc
> +sleep	1
>  guest	socat -u OPEN:test_big.bin TCP6:[__GW6__%__IFNAME__]:10002
>  nsw
>  hout	HOST_MD5_BIG md5sum __TEMP_BIG__ | cut -d' ' -f1
> @@ -179,6 +191,7 @@ check	[ "__HOST_MD5_BIG__" = "__MD5_BIG__" ]
>  
>  test	TCP/IPv6: ns to host (spliced): big transfer
>  hostb	socat -u TCP6-LISTEN:10003 OPEN:__TEMP_BIG__,create,trunc
> +sleep	1
>  ns	socat -u OPEN:__TEMP_NS_BIG__ TCP6:[::1]:10003
>  hostw
>  hout	HOST_MD5_BIG md5sum __TEMP_BIG__ | cut -d' ' -f1
> @@ -187,6 +200,7 @@ check	[ "__HOST_MD5_BIG__" = "__MD5_BIG__" ]
>  test	TCP/IPv6: ns to host (via tap): big transfer
>  hostb	socat -u TCP6-LISTEN:10003 OPEN:__TEMP_BIG__,create,trunc
>  nsout	IFNAME ip -j link show | jq -rM '.[] | select(.link_type == "ether").ifname'
> +sleep	1
>  ns	socat -u OPEN:__TEMP_NS_BIG__ TCP6:[__GW6__%__IFNAME__]:10003
>  hostw
>  hout	HOST_MD5_BIG md5sum __TEMP_BIG__ | cut -d' ' -f1
> @@ -203,6 +217,7 @@ check	[ "__GUEST_MD5_BIG__" = "__MD5_BIG__" ]
>  test	TCP/IPv6: ns to guest (using namespace address): big transfer
>  guestb	socat -u TCP6-LISTEN:10001 OPEN:test_big.bin,create,trunc
>  nsout	ADDR6 ip -j -6 addr show|jq -rM '.[] | select(.ifname == "__IFNAME__").addr_info[0].local'
> +sleep	1
>  ns	socat -u OPEN:__TEMP_NS_BIG__ TCP6:[__ADDR6__]:10001
>  guestw
>  gout	GUEST_MD5_BIG md5sum test_big.bin | cut -d' ' -f1
> @@ -218,6 +233,7 @@ check	[ "__GUEST_MD5_SMALL__" = "__MD5_SMALL__" ]
>  
>  test	TCP/IPv6: host to ns: small transfer
>  nsb	socat -u TCP6-LISTEN:10002 OPEN:__TEMP_NS_SMALL__,create,trunc
> +sleep	1
>  host	socat -u OPEN:__TEMP_SMALL__ TCP6:[::1]:10002
>  nsw
>  nsout	NS_MD5_SMALL md5sum __TEMP_NS_SMALL__ | cut -d' ' -f1
> @@ -227,6 +243,7 @@ test	TCP/IPv6: guest to host: small transfer
>  hostb	socat -u TCP6-LISTEN:10003 OPEN:__TEMP_SMALL__,create,trunc
>  gout	GW6 ip -j -6 route show|jq -rM '.[] | select(.dst == "default").gateway'
>  gout	IFNAME ip -j link show | jq -rM '.[] | select(.link_type == "ether").ifname'
> +sleep	1
>  guest	socat -u OPEN:test_small.bin TCP6:[__GW6__%__IFNAME__]:10003
>  hostw
>  hout	HOST_MD5_SMALL md5sum __TEMP_SMALL__ | cut -d' ' -f1
> @@ -234,6 +251,7 @@ check	[ "__HOST_MD5_SMALL__" = "__MD5_SMALL__" ]
>  
>  test	TCP/IPv6: guest to ns: small transfer
>  nsb	socat -u TCP6-LISTEN:10002 OPEN:__TEMP_SMALL__
> +sleep	1
>  guest	socat -u OPEN:test_small.bin TCP6:[__GW6__%__IFNAME__]:10002
>  nsw
>  hout	HOST_MD5_SMALL md5sum __TEMP_SMALL__ | cut -d' ' -f1
> @@ -241,6 +259,7 @@ check	[ "__HOST_MD5_SMALL__" = "__MD5_SMALL__" ]
>  
>  test	TCP/IPv6: ns to host (spliced): small transfer
>  hostb	socat -u TCP6-LISTEN:10003 OPEN:__TEMP_SMALL__,create,trunc
> +sleep	1
>  ns	socat -u OPEN:__TEMP_NS_SMALL__ TCP6:[::1]:10003
>  hostw
>  hout	HOST_MD5_SMALL md5sum __TEMP_SMALL__ | cut -d' ' -f1
> @@ -249,6 +268,7 @@ check	[ "__HOST_MD5_SMALL__" = "__MD5_SMALL__" ]
>  test	TCP/IPv6: ns to host (via tap): small transfer
>  hostb	socat -u TCP6-LISTEN:10003 OPEN:__TEMP_SMALL__,create,trunc
>  nsout	IFNAME ip -j link show | jq -rM '.[] | select(.link_type == "ether").ifname'
> +sleep	1
>  ns	socat -u OPEN:__TEMP_NS_SMALL__ TCP6:[__GW6__%__IFNAME__]:10003
>  hostw
>  hout	HOST_MD5_SMALL md5sum __TEMP_SMALL__ | cut -d' ' -f1
> @@ -264,6 +284,7 @@ check	[ "__GUEST_MD5_SMALL__" = "__MD5_SMALL__" ]
>  
>  test	TCP/IPv6: ns to guest (using namespace address): small transfer
>  guestb	socat -u TCP6-LISTEN:10001 OPEN:test_small.bin,create,trunc
> +sleep	1
>  ns	socat -u OPEN:__TEMP_NS_SMALL__ TCP6:[__ADDR6__]:10001
>  guestw
>  gout	GUEST_MD5_SMALL md5sum test_small.bin | cut -d' ' -f1
> diff --git a/test/passt_in_ns/udp b/test/passt_in_ns/udp
> index 5f01cbf..b7e9cf3 100644
> --- a/test/passt_in_ns/udp
> +++ b/test/passt_in_ns/udp
> @@ -18,8 +18,8 @@ htools	dd socat ip jq md5sum cut
>  test	UDP/IPv4: host to guest
>  set	TEMP __STATEDIR__/data
>  guestb	socat -u UDP4-LISTEN:10001,null-eof OPEN:test.bin,create,trunc
> -sleep	1
>  host	dd if=/dev/urandom bs=1k count=5 > __TEMP__
> +sleep	1
>  host	socat -u OPEN:__TEMP__ UDP4:127.0.0.1:10001,shut-null
>  guestw
>  hout	MD5 md5sum __TEMP__ | cut -d' ' -f1
> @@ -29,6 +29,7 @@ check	[ "__GUEST_MD5__" = "__MD5__" ]
>  test	UDP/IPv4: host to ns
>  set	TEMP_NS __STATEDIR__/data_ns
>  nsb	socat -u UDP4-LISTEN:10002,null-eof OPEN:__TEMP_NS__,create,trunc
> +sleep	1
>  host	socat -u OPEN:__TEMP__ UDP4:127.0.0.1:10002,shut-null
>  nsw
>  nsout	NS_MD5 md5sum __TEMP_NS__ | cut -d' ' -f1
> @@ -37,6 +38,7 @@ check	[ "__NS_MD5__" = "__MD5__" ]
>  test	UDP/IPv4: guest to host
>  hostb	socat -u UDP4-LISTEN:10003,null-eof OPEN:__TEMP__,create,trunc
>  gout	GW ip -j -4 route show|jq -rM '.[] | select(.dst == "default").gateway'
> +sleep	1
>  guest	socat -u OPEN:test.bin UDP4:__GW__:10003,shut-null
>  hostw
>  hout	HOST_MD5 md5sum __TEMP__ | cut -d' ' -f1
> @@ -44,14 +46,15 @@ check	[ "__HOST_MD5__" = "__MD5__" ]
>  
>  test	UDP/IPv4: guest to ns
>  nsb	socat -u UDP4-LISTEN:10002,null-eof OPEN:__TEMP_NS__,create,trunc
> +sleep	1
>  guest	socat -u OPEN:test.bin UDP4:__GW__:10002,shut-null
>  nsw
>  hout	HOST_MD5 md5sum __TEMP__ | cut -d' ' -f1
>  check	[ "__HOST_MD5__" = "__MD5__" ]
>  
>  test	UDP/IPv4: ns to host (recvmmsg/sendmmsg)
> -sleep	1
>  hostb	socat -u UDP4-LISTEN:10003,null-eof OPEN:__TEMP__,create,trunc
> +sleep	1
>  ns	socat -u OPEN:__TEMP_NS__ UDP4:127.0.0.1:10003,shut-null
>  hostw
>  hout	HOST_MD5 md5sum __TEMP__ | cut -d' ' -f1
> @@ -59,6 +62,7 @@ check	[ "__HOST_MD5__" = "__MD5__" ]
>  
>  test	UDP/IPv4: ns to host (via tap)
>  hostb	socat -u UDP4-LISTEN:10003,null-eof OPEN:__TEMP__,create,trunc
> +sleep	1
>  ns	socat -u OPEN:__TEMP_NS__ UDP4:__GW__:10003,shut-null
>  hostw
>  hout	HOST_MD5 md5sum __TEMP__ | cut -d' ' -f1
> @@ -66,6 +70,7 @@ check	[ "__HOST_MD5__" = "__MD5__" ]
>  
>  test	UDP/IPv4: ns to guest (using loopback address)
>  guestb	socat -u UDP4-LISTEN:10001,null-eof OPEN:test.bin,create,trunc
> +sleep	1
>  ns	socat -u OPEN:__TEMP_NS__ UDP4:127.0.0.1:10001,shut-null
>  guestw
>  gout	GUEST_MD5 md5sum test.bin | cut -d' ' -f1
> @@ -75,6 +80,7 @@ test	UDP/IPv4: ns to guest (using namespace address)
>  guestb	socat -u UDP4-LISTEN:10001,null-eof OPEN:test.bin,create,trunc
>  nsout	IFNAME ip -j link show | jq -rM '.[] | select(.link_type == "ether").ifname'
>  nsout	ADDR ip -j -4 addr show|jq -rM '.[] | select(.ifname == "__IFNAME__").addr_info[0].local'
> +sleep	1
>  ns	socat -u OPEN:__TEMP_NS__ UDP4:__ADDR__:10001,shut-null
>  guestw
>  gout	GUEST_MD5 md5sum test.bin | cut -d' ' -f1
> @@ -82,6 +88,7 @@ check	[ "__GUEST_MD5__" = "__MD5__" ]
>  
>  test	UDP/IPv6: host to guest
>  guestb	socat -u UDP6-LISTEN:10001,null-eof OPEN:test.bin,create,trunc
> +sleep	1
>  host	socat -u OPEN:__TEMP__ UDP6:[::1]:10001,shut-null
>  guestw
>  gout	GUEST_MD5 md5sum test.bin | cut -d' ' -f1
> @@ -89,6 +96,7 @@ check	[ "__GUEST_MD5__" = "__MD5__" ]
>  
>  test	UDP/IPv6: host to ns
>  nsb	socat -u UDP6-LISTEN:10002,null-eof OPEN:__TEMP_NS__,create,trunc
> +sleep	1
>  host	socat -u OPEN:__TEMP__ UDP6:[::1]:10002,shut-null
>  nsw
>  nsout	NS_MD5 md5sum __TEMP_NS__ | cut -d' ' -f1
> @@ -98,6 +106,7 @@ test	UDP/IPv6: guest to host
>  hostb	socat -u UDP6-LISTEN:10003,null-eof OPEN:__TEMP__,create,trunc
>  gout	GW6 ip -j -6 route show|jq -rM '.[] | select(.dst == "default").gateway'
>  gout	IFNAME ip -j link show | jq -rM '.[] | select(.link_type == "ether").ifname'
> +sleep	1
>  guest	socat -u OPEN:test.bin UDP6:[__GW6__%__IFNAME__]:10003,shut-null
>  hostw
>  hout	HOST_MD5 md5sum __TEMP__ | cut -d' ' -f1
> @@ -105,6 +114,7 @@ check	[ "__HOST_MD5__" = "__MD5__" ]
>  
>  test	UDP/IPv6: guest to ns
>  nsb	socat -u UDP6-LISTEN:10002,null-eof OPEN:__TEMP_NS__,create,trunc
> +sleep	1
>  guest	socat -u OPEN:test.bin UDP6:[__GW6__%__IFNAME__]:10002,shut-null
>  nsw
>  hout	HOST_MD5 md5sum __TEMP__ | cut -d' ' -f1
> @@ -112,6 +122,7 @@ check	[ "__HOST_MD5__" = "__MD5__" ]
>  
>  test	UDP/IPv6: ns to host (recvmmsg/sendmmsg)
>  hostb	socat -u UDP6-LISTEN:10003,null-eof OPEN:__TEMP__,create,trunc
> +sleep	1
>  ns	socat -u OPEN:__TEMP_NS__ UDP6:[::1]:10003,shut-null
>  hostw
>  hout	HOST_MD5 md5sum __TEMP__ | cut -d' ' -f1
> @@ -120,6 +131,7 @@ check	[ "__HOST_MD5__" = "__MD5__" ]
>  test	UDP/IPv6: ns to host (via tap)
>  hostb	socat -u UDP6-LISTEN:10003,null-eof OPEN:__TEMP__,create,trunc
>  nsout	IFNAME ip -j link show | jq -rM '.[] | select(.link_type == "ether").ifname'
> +sleep	1
>  ns	socat -u OPEN:__TEMP_NS__ UDP6:[__GW6__%__IFNAME__]:10003,shut-null
>  hostw
>  hout	HOST_MD5 md5sum __TEMP__ | cut -d' ' -f1
> @@ -127,6 +139,7 @@ check	[ "__HOST_MD5__" = "__MD5__" ]
>  
>  test	UDP/IPv6: ns to guest (using loopback address)
>  guestb	socat -u UDP6-LISTEN:10001,null-eof OPEN:test.bin,create,trunc
> +sleep	1
>  ns	socat -u OPEN:__TEMP_NS__ UDP6:[::1]:10001,shut-null
>  guestw
>  gout	GUEST_MD5 md5sum test.bin | cut -d' ' -f1
> @@ -135,6 +148,7 @@ check	[ "__GUEST_MD5__" = "__MD5__" ]
>  test	UDP/IPv6: ns to guest (using namespace address)
>  guestb	socat -u UDP6-LISTEN:10001,null-eof OPEN:test.bin,create,trunc
>  nsout	ADDR6 ip -j -6 addr show|jq -rM '.[] | select(.ifname == "__IFNAME__").addr_info[0].local'
> +sleep	1
>  ns	socat -u OPEN:__TEMP_NS__ UDP6:[__ADDR6__]:10001,shut-null
>  guestw
>  gout	GUEST_MD5 md5sum test.bin | cut -d' ' -f1

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/2] test/perf: Check for /sbin/sysctl with which(1), not simply sysctl

[Stefano Brivio]

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

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

> On Sat, Sep 17, 2022 at 01:55:33AM +0200, Stefano Brivio wrote:
> > Otherwise, we're depending on having /sbin in $PATH. For some reason  
> 
> Huh.  I wonder why this is working for me.

Different shells, or different shell profiles, I guess?

> > I didn't completely grasp, with the new command dispatch mechanism
> > that's not the case anymore, even if I have /sbin in $PATH in the
> > parent shell.  
> 
> For the guest, there's no mystery - there's no strong connection
> between the shell environments there.  We probably can control PATH
> with our mbuto setup though.
> 
> For the ns that is strange, though.

I think the reason is that with:

        echo "nsenter $@ sh -c" > "${__enter}"

you get a different behaviour than nsenter with an interactive shell
(previous implementation). For example, on Debian, commands (by default)
will run with dash instead of bash.

In any case, just using 'which sysctl' looks wrong to me, and that can
break for many other reasons, so I don't feel very inclined in digging
into the actual reason and differences between environments here.

-- 
Stefano

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

[Stefano Brivio]

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

Re: [PATCH 1/2] test/perf: Check for /sbin/sysctl with which(1), not simply sysctl

[David Gibson]

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

On Sat, Sep 17, 2022 at 10:44:36AM +0200, Stefano Brivio wrote:
> On Sat, 17 Sep 2022 13:27:32 +1000
> David Gibson <david(a)gibson.dropbear.id.au> wrote:
> 
> > On Sat, Sep 17, 2022 at 01:55:33AM +0200, Stefano Brivio wrote:
> > > Otherwise, we're depending on having /sbin in $PATH. For some reason  
> > 
> > Huh.  I wonder why this is working for me.
> 
> Different shells, or different shell profiles, I guess?
> 
> > > I didn't completely grasp, with the new command dispatch mechanism
> > > that's not the case anymore, even if I have /sbin in $PATH in the
> > > parent shell.  
> > 
> > For the guest, there's no mystery - there's no strong connection
> > between the shell environments there.  We probably can control PATH
> > with our mbuto setup though.
> > 
> > For the ns that is strange, though.
> 
> I think the reason is that with:
> 
>         echo "nsenter $@ sh -c" > "${__enter}"
> 
> you get a different behaviour than nsenter with an interactive shell
> (previous implementation). For example, on Debian, commands (by default)
> will run with dash instead of bash.
> 
> In any case, just using 'which sysctl' looks wrong to me, and that can
> break for many other reasons, so I don't feel very inclined in digging
> into the actual reason and differences between environments here.

Yeah, fair enough.

Reviewed-by: David Gibson <david(a)gibson.dropbear.id.au>

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[David Gibson]

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

On Sat, Sep 17, 2022 at 10:44:41AM +0200, Stefano Brivio wrote:
> 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

Ah, right.

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

Yeah, I suppose.

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

Yeah, fair enough.  When I rebase I'll see if there's any refinements
I can make relatively easily.

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

I'm not entirely sure what you mean by manual retries.  I was trying
to use socat's retry option, but that only seems to fire on
ECONNREFUSED.

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

Right, I found that... but I'm not sure it is sending an RST.  Setting
SO_LINGER with zero timeout does send an RST, but I'm not sure it does
so without it.

Even if it does, it's not really helpful for this.  I found that
sending an RST doesn't reliably cause the client socat to exit with
failure.  I'm not 100% sure what's going on, but I think what can
happen is that the client sends everything and completes before the
RST arrives, because it all gets buffered before in passt (or at least
in the kernel socket buffers associated with passt).  In that case the
client doesn't detect the error.

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

Yeah, about 36 minutes to be precise, I think.  There's a reason I
haven't been attempting to run this until now.

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

Huh, interesting.  I wonder if it's because it simply isn't finding
all the system headers for you.  I see you have a missingIncludeSystem
suppression in there, and when I removed that I found it complained
about not finding the headers on Debian until I added
-I/usr/include/x86_64-linux-gnu.  Anyway, I have patches that fix this
which I'll be sending soon.  In any case, since the run time will be
exponential in the number of config defines, it doesn't take a huge
difference in the headers to make a vast difference to runtime.

I also found that both clang-tidy and cppcheck fail for me, I have a
bunch of fixes for this, but I'm not finished yet.  There's a couple
of tricky ones, including one with dueling errors - cppcheck says
there's a redundant NULL check, but if I remove it clang-tidy says
there's a NULL pointer dereference.  Still working on it.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Stefano Brivio]

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

On Sat, 17 Sep 2022 20:19:21 +1000
David Gibson <david(a)gibson.dropbear.id.au> wrote:

> On Sat, Sep 17, 2022 at 10:44:41AM +0200, Stefano Brivio wrote:
> > 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  
> 
> Ah, right.
> 
> > > 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.  
> 
> Yeah, I suppose.
> 
> > 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.  
> 
> Yeah, fair enough.  When I rebase I'll see if there's any refinements
> I can make relatively easily.

Okay, thanks.

> > > 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,  
> 
> I'm not entirely sure what you mean by manual retries.  I was trying
> to use socat's retry option, but that only seems to fire on
> ECONNREFUSED.

I was watching the text execution, then as I saw the socat server
process getting stuck, I just re-run the client with ssh -F ... with the
original command, and that worked for me every time.

So, the behaviour I'm inferring is:

- server not listening, no retry option: connect() goes
  through, but anything else will get ECONNRESET (RST from passt), plus
  there should be EPOLLERR if socat checks that: client exits

- server not listening, retry option: client exits without retry because
  it doesn't get ECONNREFUSED

...I guess a cleaner behaviour on passt's side would be to delay the
accept() (that's in tcp_conn_from_sock()) until we get SYN, ACK from
the guest. But:

- we can't send a RST if we don't accept(), as we have no socket to
  close(). Maybe we could close and reopen the listening socket...? We
  need to be a bit careful about not turning that into a vector for
  DoS, though

- we can't send ICMP/ICMPv6 messages

...so we risk a connect() timeout, which is even less desirable.

In case the connection goes through, though... I actually tried in the
past to wait before we accept(), and it was increasing latency (of
course), so I discarded that approach, because I couldn't think of any
practical case.

But here we are, so perhaps this behaviour should be there, at least as
an option (maybe even as default).

> > 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.  
> 
> Right, I found that... but I'm not sure it is sending an RST.  Setting
> SO_LINGER with zero timeout does send an RST, but I'm not sure it does
> so without it.

I actually checked, it did for me, without SO_LINGER.

> Even if it does, it's not really helpful for this.  I found that
> sending an RST doesn't reliably cause the client socat to exit with
> failure.  I'm not 100% sure what's going on, but I think what can
> happen is that the client sends everything and completes before the
> RST arrives, because it all gets buffered before in passt (or at least
> in the kernel socket buffers associated with passt).  In that case the
> client doesn't detect the error.

...right.

> > 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,  
> 
> Yeah, about 36 minutes to be precise, I think.  There's a reason I
> haven't been attempting to run this until now.

Wow, okay, I had no idea.

> > 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. :)  
> 
> Huh, interesting.  I wonder if it's because it simply isn't finding
> all the system headers for you.  I see you have a missingIncludeSystem
> suppression in there, and when I removed that I found it complained
> about not finding the headers on Debian until I added
> -I/usr/include/x86_64-linux-gnu.  Anyway, I have patches that fix this
> which I'll be sending soon.  In any case, since the run time will be
> exponential in the number of config defines, it doesn't take a huge
> difference in the headers to make a vast difference to runtime.

Ah, yes, that might explain it.

> I also found that both clang-tidy and cppcheck fail for me, I have a
> bunch of fixes for this, but I'm not finished yet.  There's a couple
> of tricky ones, including one with dueling errors - cppcheck says
> there's a redundant NULL check, but if I remove it clang-tidy says
> there's a NULL pointer dereference.  Still working on it.

I'm currently using Cppcheck 2.8 and LLVM 13.0.1, perhaps you have more
recent versions, I'll update them as soon as I finally get the tests to
go through. ;) Thanks in advance for fixing those.

-- 
Stefano

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

[David Gibson]

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

On Sat, Sep 17, 2022 at 01:28:42PM +0200, Stefano Brivio wrote:
> On Sat, 17 Sep 2022 20:19:21 +1000
> David Gibson <david(a)gibson.dropbear.id.au> wrote:
> 
> > On Sat, Sep 17, 2022 at 10:44:41AM +0200, Stefano Brivio wrote:
> > > 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  
> > 
> > Ah, right.
> > 
> > > > 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.  
> > 
> > Yeah, I suppose.
> > 
> > > 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.  
> > 
> > Yeah, fair enough.  When I rebase I'll see if there's any refinements
> > I can make relatively easily.
> 
> Okay, thanks.
> 
> > > > 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,  
> > 
> > I'm not entirely sure what you mean by manual retries.  I was trying
> > to use socat's retry option, but that only seems to fire on
> > ECONNREFUSED.
> 
> I was watching the text execution, then as I saw the socat server
> process getting stuck, I just re-run the client with ssh -F ... with the
> original command, and that worked for me every time.

Ah, yeah, I'd expect that to work.  The difficulty is automatically
detecting the failure.

> So, the behaviour I'm inferring is:
> 
> - server not listening, no retry option: connect() goes
>   through, but anything else will get ECONNRESET (RST from passt), plus
>   there should be EPOLLERR if socat checks that: client exits

That doesn't seem to be the case.  We *eventually* get an ECONNRESET,
but it seems like we can send some data before it happens - sometimes
quite a lot.  I'm assuming this is because quite a lot can fit into
socket buffers before the RST makes its way back.

> - server not listening, retry option: client exits without retry because
>   it doesn't get ECONNREFUSED

Right.

> ...I guess a cleaner behaviour on passt's side would be to delay the
> accept() (that's in tcp_conn_from_sock()) until we get SYN, ACK from
> the guest. But:

I thought about that, but I don't think its workable.  For starters, I
don't think there's a way of explicitly refusing a connection from
userspace.  Not accept()ing, then closing and re-opening the listening
socket *might* do it.

But.. more fundamentally, due to the connection backlog, I think by
the time we get the notification in userspace, the kernel has probably
already completed the TCP handshake, so it's too late to signal a
connection refused to the client.

> - we can't send a RST if we don't accept(), as we have no socket to
>   close(). Maybe we could close and reopen the listening socket...? We
>   need to be a bit careful about not turning that into a vector for
>   DoS, though
> 
> - we can't send ICMP/ICMPv6 messages
> 
> ...so we risk a connect() timeout, which is even less desirable.
> 
> In case the connection goes through, though... I actually tried in the
> past to wait before we accept(), and it was increasing latency (of
> course), so I discarded that approach, because I couldn't think of any
> practical case.
> 
> But here we are, so perhaps this behaviour should be there, at least as
> an option (maybe even as default).
> 
> > > 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.  
> > 
> > Right, I found that... but I'm not sure it is sending an RST.  Setting
> > SO_LINGER with zero timeout does send an RST, but I'm not sure it does
> > so without it.
> 
> I actually checked, it did for me, without SO_LINGER.
> 
> > Even if it does, it's not really helpful for this.  I found that
> > sending an RST doesn't reliably cause the client socat to exit with
> > failure.  I'm not 100% sure what's going on, but I think what can
> > happen is that the client sends everything and completes before the
> > RST arrives, because it all gets buffered before in passt (or at least
> > in the kernel socket buffers associated with passt).  In that case the
> > client doesn't detect the error.
> 
> ...right.
> 
> > > 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,  
> > 
> > Yeah, about 36 minutes to be precise, I think.  There's a reason I
> > haven't been attempting to run this until now.
> 
> Wow, okay, I had no idea.
> 
> > > 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. :)  
> > 
> > Huh, interesting.  I wonder if it's because it simply isn't finding
> > all the system headers for you.  I see you have a missingIncludeSystem
> > suppression in there, and when I removed that I found it complained
> > about not finding the headers on Debian until I added
> > -I/usr/include/x86_64-linux-gnu.  Anyway, I have patches that fix this
> > which I'll be sending soon.  In any case, since the run time will be
> > exponential in the number of config defines, it doesn't take a huge
> > difference in the headers to make a vast difference to runtime.
> 
> Ah, yes, that might explain it.
> 
> > I also found that both clang-tidy and cppcheck fail for me, I have a
> > bunch of fixes for this, but I'm not finished yet.  There's a couple
> > of tricky ones, including one with dueling errors - cppcheck says
> > there's a redundant NULL check, but if I remove it clang-tidy says
> > there's a NULL pointer dereference.  Still working on it.
> 
> I'm currently using Cppcheck 2.8 and LLVM 13.0.1, perhaps you have more
> recent versions, I'll update them as soon as I finally get the tests to
> go through. ;) Thanks in advance for fixing those.

Huh, odd.  I'm using cppcheck 2.7 most of the time (the one packaged
in Fedora 36).  I also tried cppcheck 2.9 on Debian which gets some
additional errors.  I have LLVM 14.0.5, so that might explain that
side.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Stefano Brivio]

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

On Mon, 19 Sep 2022 11:48:33 +1000
David Gibson <david(a)gibson.dropbear.id.au> wrote:

> On Sat, Sep 17, 2022 at 01:28:42PM +0200, Stefano Brivio wrote:
> > On Sat, 17 Sep 2022 20:19:21 +1000
> > David Gibson <david(a)gibson.dropbear.id.au> wrote:
> >   
> > > On Sat, Sep 17, 2022 at 10:44:41AM +0200, Stefano Brivio wrote:  
> > > > 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    
> > > 
> > > Ah, right.
> > >   
> > > > > 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.    
> > > 
> > > Yeah, I suppose.
> > >   
> > > > 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.    
> > > 
> > > Yeah, fair enough.  When I rebase I'll see if there's any refinements
> > > I can make relatively easily.  
> > 
> > Okay, thanks.
> >   
> > > > > 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,    
> > > 
> > > I'm not entirely sure what you mean by manual retries.  I was trying
> > > to use socat's retry option, but that only seems to fire on
> > > ECONNREFUSED.  
> > 
> > I was watching the text execution, then as I saw the socat server
> > process getting stuck, I just re-run the client with ssh -F ... with the
> > original command, and that worked for me every time.  
> 
> Ah, yeah, I'd expect that to work.  The difficulty is automatically
> detecting the failure.
> 
> > So, the behaviour I'm inferring is:
> > 
> > - server not listening, no retry option: connect() goes
> >   through, but anything else will get ECONNRESET (RST from passt), plus
> >   there should be EPOLLERR if socat checks that: client exits  
> 
> That doesn't seem to be the case.  We *eventually* get an ECONNRESET,
> but it seems like we can send some data before it happens - sometimes
> quite a lot.  I'm assuming this is because quite a lot can fit into
> socket buffers before the RST makes its way back.

Well, yes, it might simply be that socat relies on ECONNRESET from
send(), and the first send()-like call includes the full buffer.

Or, even if it's already checking for EPOLLERR at that point, it could
be delivered too late.

> > - server not listening, retry option: client exits without retry because
> >   it doesn't get ECONNREFUSED  
> 
> Right.
> 
> > ...I guess a cleaner behaviour on passt's side would be to delay the
> > accept() (that's in tcp_conn_from_sock()) until we get SYN, ACK from
> > the guest. But:  
> 
> I thought about that, but I don't think its workable.  For starters, I
> don't think there's a way of explicitly refusing a connection from
> userspace.  Not accept()ing, then closing and re-opening the listening
> socket *might* do it.
> 
> But.. more fundamentally, due to the connection backlog, I think by
> the time we get the notification in userspace, the kernel has probably
> already completed the TCP handshake, so it's too late to signal a
> connection refused to the client.

Oops, right, listen() by itself already warrants a handshake, so I
guess there's no point in trying if we can't ensure a given behaviour.

In real-world applications:

- the user forwards a set of ports because they expect a server to be
  reachable on those ports, fine

- or all ports are forwarded (this is the case of KubeVirt) so that the
  user doesn't have to configure them. In that case, we can always have
  handshake, client sending data, and receiving a RST before any of it
  is acknowledged -- this is guaranteed by passt as it won't dequeue
  data from buffers before the real server accepted the connection

...the only alternatives I see sound a lot like a port scan, which is
definitely not desirable.

I guess we just have to... accept this behaviour. The important fact, I
think, is that we don't acknowledge data.

Whether closing and re-opening the listening socket causes ICMP
messages to be sent: from kernel code I'd say it's not the case, but it
might be worth an actual test, as that would be a marginal improvement.

-- 
Stefano

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

[David Gibson]

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

On Mon, Sep 19, 2022 at 08:41:50AM +0200, Stefano Brivio wrote:
> On Mon, 19 Sep 2022 11:48:33 +1000
> David Gibson <david(a)gibson.dropbear.id.au> wrote:
> 
> > On Sat, Sep 17, 2022 at 01:28:42PM +0200, Stefano Brivio wrote:
> > > On Sat, 17 Sep 2022 20:19:21 +1000
> > > David Gibson <david(a)gibson.dropbear.id.au> wrote:
> > >   
> > > > On Sat, Sep 17, 2022 at 10:44:41AM +0200, Stefano Brivio wrote:  
> > > > > 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    
> > > > 
> > > > Ah, right.
> > > >   
> > > > > > 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.    
> > > > 
> > > > Yeah, I suppose.
> > > >   
> > > > > 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.    
> > > > 
> > > > Yeah, fair enough.  When I rebase I'll see if there's any refinements
> > > > I can make relatively easily.  
> > > 
> > > Okay, thanks.
> > >   
> > > > > > 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,    
> > > > 
> > > > I'm not entirely sure what you mean by manual retries.  I was trying
> > > > to use socat's retry option, but that only seems to fire on
> > > > ECONNREFUSED.  
> > > 
> > > I was watching the text execution, then as I saw the socat server
> > > process getting stuck, I just re-run the client with ssh -F ... with the
> > > original command, and that worked for me every time.  
> > 
> > Ah, yeah, I'd expect that to work.  The difficulty is automatically
> > detecting the failure.
> > 
> > > So, the behaviour I'm inferring is:
> > > 
> > > - server not listening, no retry option: connect() goes
> > >   through, but anything else will get ECONNRESET (RST from passt), plus
> > >   there should be EPOLLERR if socat checks that: client exits  
> > 
> > That doesn't seem to be the case.  We *eventually* get an ECONNRESET,
> > but it seems like we can send some data before it happens - sometimes
> > quite a lot.  I'm assuming this is because quite a lot can fit into
> > socket buffers before the RST makes its way back.
> 
> Well, yes, it might simply be that socat relies on ECONNRESET from
> send(), and the first send()-like call includes the full buffer.

Yeah, that appears to be the case.  When it did fail, it seemed to be
because it was getting an ECONNRESET on one of the send()s or
write()s.

> Or, even if it's already checking for EPOLLERR at that point, it could
> be delivered too late.
> 
> > > - server not listening, retry option: client exits without retry because
> > >   it doesn't get ECONNREFUSED  
> > 
> > Right.
> > 
> > > ...I guess a cleaner behaviour on passt's side would be to delay the
> > > accept() (that's in tcp_conn_from_sock()) until we get SYN, ACK from
> > > the guest. But:  
> > 
> > I thought about that, but I don't think its workable.  For starters, I
> > don't think there's a way of explicitly refusing a connection from
> > userspace.  Not accept()ing, then closing and re-opening the listening
> > socket *might* do it.
> > 
> > But.. more fundamentally, due to the connection backlog, I think by
> > the time we get the notification in userspace, the kernel has probably
> > already completed the TCP handshake, so it's too late to signal a
> > connection refused to the client.
> 
> Oops, right, listen() by itself already warrants a handshake, so I
> guess there's no point in trying if we can't ensure a given behaviour.
> 
> In real-world applications:
> 
> - the user forwards a set of ports because they expect a server to be
>   reachable on those ports, fine
> 
> - or all ports are forwarded (this is the case of KubeVirt) so that the
>   user doesn't have to configure them. In that case, we can always have
>   handshake, client sending data, and receiving a RST before any of it
>   is acknowledged -- this is guaranteed by passt as it won't dequeue
>   data from buffers before the real server accepted the connection

Uuuhh.. does that actually guarantee that?  Does the kernel send an
ack only once userspace has read the data, or just once it is queued
in-kernel?

> ...the only alternatives I see sound a lot like a port scan, which is
> definitely not desirable.

Right.

> I guess we just have to... accept this behaviour.

Yeah, I think so.

> The important fact, I
> think, is that we don't acknowledge data.
> 
> Whether closing and re-opening the listening socket causes ICMP
> messages to be sent: from kernel code I'd say it's not the case, but it
> might be worth an actual test, as that would be a marginal improvement.

Ok.  It's not really clear to me what ICMP messages would be sent to
where in that scenario.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Stefano Brivio]

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

On Mon, 19 Sep 2022 17:00:23 +1000
David Gibson <david(a)gibson.dropbear.id.au> wrote:

> On Mon, Sep 19, 2022 at 08:41:50AM +0200, Stefano Brivio wrote:
> > On Mon, 19 Sep 2022 11:48:33 +1000
> > David Gibson <david(a)gibson.dropbear.id.au> wrote:
> >   
> > > On Sat, Sep 17, 2022 at 01:28:42PM +0200, Stefano Brivio wrote:  
> > > > On Sat, 17 Sep 2022 20:19:21 +1000
> > > > David Gibson <david(a)gibson.dropbear.id.au> wrote:
> > > >     
> > > > > On Sat, Sep 17, 2022 at 10:44:41AM +0200, Stefano Brivio wrote:    
> > > > > > 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      
> > > > > 
> > > > > Ah, right.
> > > > >     
> > > > > > > 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.      
> > > > > 
> > > > > Yeah, I suppose.
> > > > >     
> > > > > > 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.      
> > > > > 
> > > > > Yeah, fair enough.  When I rebase I'll see if there's any refinements
> > > > > I can make relatively easily.    
> > > > 
> > > > Okay, thanks.
> > > >     
> > > > > > > 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,      
> > > > > 
> > > > > I'm not entirely sure what you mean by manual retries.  I was trying
> > > > > to use socat's retry option, but that only seems to fire on
> > > > > ECONNREFUSED.    
> > > > 
> > > > I was watching the text execution, then as I saw the socat server
> > > > process getting stuck, I just re-run the client with ssh -F ... with the
> > > > original command, and that worked for me every time.    
> > > 
> > > Ah, yeah, I'd expect that to work.  The difficulty is automatically
> > > detecting the failure.
> > >   
> > > > So, the behaviour I'm inferring is:
> > > > 
> > > > - server not listening, no retry option: connect() goes
> > > >   through, but anything else will get ECONNRESET (RST from passt), plus
> > > >   there should be EPOLLERR if socat checks that: client exits    
> > > 
> > > That doesn't seem to be the case.  We *eventually* get an ECONNRESET,
> > > but it seems like we can send some data before it happens - sometimes
> > > quite a lot.  I'm assuming this is because quite a lot can fit into
> > > socket buffers before the RST makes its way back.  
> > 
> > Well, yes, it might simply be that socat relies on ECONNRESET from
> > send(), and the first send()-like call includes the full buffer.  
> 
> Yeah, that appears to be the case.  When it did fail, it seemed to be
> because it was getting an ECONNRESET on one of the send()s or
> write()s.
> 
> > Or, even if it's already checking for EPOLLERR at that point, it could
> > be delivered too late.
> >   
> > > > - server not listening, retry option: client exits without retry because
> > > >   it doesn't get ECONNREFUSED    
> > > 
> > > Right.
> > >   
> > > > ...I guess a cleaner behaviour on passt's side would be to delay the
> > > > accept() (that's in tcp_conn_from_sock()) until we get SYN, ACK from
> > > > the guest. But:    
> > > 
> > > I thought about that, but I don't think its workable.  For starters, I
> > > don't think there's a way of explicitly refusing a connection from
> > > userspace.  Not accept()ing, then closing and re-opening the listening
> > > socket *might* do it.
> > > 
> > > But.. more fundamentally, due to the connection backlog, I think by
> > > the time we get the notification in userspace, the kernel has probably
> > > already completed the TCP handshake, so it's too late to signal a
> > > connection refused to the client.  
> > 
> > Oops, right, listen() by itself already warrants a handshake, so I
> > guess there's no point in trying if we can't ensure a given behaviour.
> > 
> > In real-world applications:
> > 
> > - the user forwards a set of ports because they expect a server to be
> >   reachable on those ports, fine
> > 
> > - or all ports are forwarded (this is the case of KubeVirt) so that the
> >   user doesn't have to configure them. In that case, we can always have
> >   handshake, client sending data, and receiving a RST before any of it
> >   is acknowledged -- this is guaranteed by passt as it won't dequeue
> >   data from buffers before the real server accepted the connection  
> 
> Uuuhh.. does that actually guarantee that?  Does the kernel send an
> ack only once userspace has read the data, or just once it is queued
> in-kernel?

As far as I remember, for the first segment the condition
__tcp_select_window(sk) >= tp->rcv_wnd in __tcp_ack_snd_check(),
net/ipv4/tcp_input.c, should be false, and if more data is sent and we
don't dequeue it, the window doesn't advance, so the first ACK _should_
be deferred to the first tcp_recvmsg().

However, this is a bit vague now in my memory. I guess we should
double check this (by delaying the first recvmsg() in passt and
comparing with captures), and also what happens if we defer the accept()
to after the connection is established guest-side.

> > ...the only alternatives I see sound a lot like a port scan, which is
> > definitely not desirable.  
> 
> Right.
> 
> > I guess we just have to... accept this behaviour.  
> 
> Yeah, I think so.
> 
> > The important fact, I
> > think, is that we don't acknowledge data.
> > 
> > Whether closing and re-opening the listening socket causes ICMP
> > messages to be sent: from kernel code I'd say it's not the case, but it
> > might be worth an actual test, as that would be a marginal improvement.  
> 
> Ok.  It's not really clear to me what ICMP messages would be sent to
> where in that scenario.

Hmm... for UDP, ICMP type 3 code 3 (port unreachable) and ICMPv6 type 1
code 3 (also port unreachable) are sent by the kernel to the source
address of the SYN segment if there's no corresponding socket.

For TCP, they could legitimately be sent ("may", in RFC 792), but now
that I think about it, I can't recall the kernel doing it... unless
netfilter is explicitly configured to do so. So this is probably
irrelevant (also perhaps worth a quick check anyway).

-- 
Stefano