On 1 Sep 2022, at 14:11, David Marchand wrote:
> On Wed, Aug 31, 2022 at 1:21 PM Eelco Chaudron <[email protected]> wrote: >> >> Using NETNS_DAEMONIZE will start tcpdump in the background, and it will >> also make sure it gets killed in corner cases. >> >> For the check_pkt_len tests, we also kill tcpdump between individual >> tests in the same test case to avoid confusion when analyzing results. >> >> Fixes: 02dabb21f243 ("tests: Add check_pkt_len action test to >> system-offload-traffic.") >> Suggested-by: [email protected] > > I am not sure I suggested to cleanup everything, but if you think I > deserve some credit, I prefer with a full name :-). > Suggested-by: David Marchand <[email protected]> ACK, you suggested killing tcpdump between test cases. Not sure why your full name got lost, as it was there in V1 > >> Signed-off-by: Eelco Chaudron <[email protected]> >> --- >> v2: >> - Replaced NS_CHECK_EXEC with NETNS_DAEMONIZE for all tcpdump use cases. > > I spotted two remaining tcpdump started in background for some conntrack test. > Worth fixing, from my pov. Guess my grep skills were not good enough, will fixed in v3. In addition, I found a lot more additional ones, will fix those also… >> >> tests/system-offloads-traffic.at | 28 ++++++++++++++++------------ >> tests/system-traffic.at | 22 +++++++++++----------- >> 2 files changed, 27 insertions(+), 23 deletions(-) >> >> diff --git a/tests/system-offloads-traffic.at >> b/tests/system-offloads-traffic.at >> index 1e1012965..d9b815a5d 100644 >> --- a/tests/system-offloads-traffic.at >> +++ b/tests/system-offloads-traffic.at >> @@ -297,8 +297,8 @@ table=4,in_port=1,reg0=0x0 actions=output:4 >> ]) >> AT_CHECK([ovs-ofctl --protocols=OpenFlow10 add-flows br0 flows.txt]) >> >> -NS_CHECK_EXEC([at_ns3], [tcpdump -l -n -U -i p3 dst 10.1.1.2 and icmp > >> p3.pcap 2>/dev/null &]) >> -NS_CHECK_EXEC([at_ns4], [tcpdump -l -n -U -i p4 dst 10.1.1.2 and icmp > >> p4.pcap 2>/dev/null &]) >> +NETNS_DAEMONIZE([at_ns3], [tcpdump -l -n -U -i p3 dst 10.1.1.2 and icmp > >> p3.pcap 2>/dev/null], [tcpdump3.pid]) >> +NETNS_DAEMONIZE([at_ns4], [tcpdump -l -n -U -i p4 dst 10.1.1.2 and icmp > >> p4.pcap 2>/dev/null], [tcpdump4.pid]) >> sleep 1 >> >> NS_CHECK_EXEC([at_ns1], [ping -q -c 10 -i 0.1 -w 2 -s 64 10.1.1.2 | >> FORMAT_PING], [0], [dnl >> @@ -325,10 +325,10 @@ AT_CHECK([test $(ovs-appctl upcall/show | grep -c >> "offloaded flows") -eq 0], [0] >> >> OVS_TRAFFIC_VSWITCHD_STOP >> >> -AT_CHECK([cat p3.pcap | awk '{print $NF}' | uniq -c | awk '{$1=$1;print}'], >> [0], [dnl >> +AT_CHECK([cat p3.pcap | awk 'NF{print $NF}' | uniq -c | awk >> '{$1=$1;print}'], [0], [dnl > > I don't see the relation with $subject. > Why would we need to filter out empty lines? Stopping tcpdump will add extra newlines to the output. Will add this to the commit message in v3. >> 10 1032 >> ]) >> -AT_CHECK([cat p4.pcap | awk '{print $NF}' | uniq -c | awk '{$1=$1;print}'], >> [0], [dnl >> +AT_CHECK([cat p4.pcap | awk 'NF{print $NF}' | uniq -c | awk >> '{$1=$1;print}'], [0], [dnl >> 10 72 >> ]) >> >> @@ -355,8 +355,8 @@ table=4,in_port=1,reg0=0x0 actions=output:4 >> ]) >> AT_CHECK([ovs-ofctl --protocols=OpenFlow10 add-flows br0 flows.txt]) >> >> -NS_CHECK_EXEC([at_ns3], [tcpdump -l -n -U -i p3 dst 10.1.1.2 and icmp > >> p3.pcap 2>/dev/null &]) >> -NS_CHECK_EXEC([at_ns4], [tcpdump -l -n -U -i p4 dst 10.1.1.2 and icmp > >> p4.pcap 2>/dev/null &]) >> +NETNS_DAEMONIZE([at_ns3], [tcpdump -l -n -U -i p3 dst 10.1.1.2 and icmp > >> p3.pcap 2>/dev/null], [tcpdump3.pid]) >> +NETNS_DAEMONIZE([at_ns4], [tcpdump -l -n -U -i p4 dst 10.1.1.2 and icmp > >> p4.pcap 2>/dev/null], [tcpdump4.pid]) >> sleep 1 >> >> NS_CHECK_EXEC([at_ns1], [ping -q -c 10 -i 0.1 -w 2 -s 64 10.1.1.2 | >> FORMAT_PING], [0], [dnl >> @@ -382,10 +382,12 @@ in_port(3),eth(),eth_type(0x0800),ipv4(frag=no), >> packets:19, bytes:11614, used:0 >> AT_CHECK([ovs-appctl upcall/show | grep -E "offloaded flows : [[1-9]]"], >> [0], [ignore]) >> >> sleep 1 >> -AT_CHECK([cat p3.pcap | awk '{print $NF}' | uniq -c | awk '{$1=$1;print}'], >> [0], [dnl >> +kill $(cat tcpdump3.pid) >> +kill $(cat tcpdump4.pid) > > Looks ok to me. > It is really unlikely those pid numbers will get reused by the time > this test cleanup code is invoked, so no need to flush tcpdump*.pid > files. Yes, I assume if this happens there is something else wrong with your environment :) >> +AT_CHECK([cat p3.pcap | awk 'NF{print $NF}' | uniq -c | awk >> '{$1=$1;print}'], [0], [dnl >> 10 1032 >> ]) >> -AT_CHECK([cat p4.pcap | awk '{print $NF}' | uniq -c | awk '{$1=$1;print}'], >> [0], [dnl >> +AT_CHECK([cat p4.pcap | awk 'NF{print $NF}' | uniq -c | awk >> '{$1=$1;print}'], [0], [dnl >> 10 72 >> ]) >> > > > -- > David Marchand _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
