On 1/7/26 5:09 PM, David Marchand wrote: > On Wed, 7 Jan 2026 at 13:41, Ilya Maximets <[email protected]> wrote: >> >> On 12/15/25 2:57 PM, David Marchand wrote: >>> Define macros to limit copy/paste (those will be reused later). >>> >>> To ease test debugging, store composed and expected good/bad packets >>> in the unit test working directory, and pass the file names. >>> >>> Example: >>> $ make -C build check TESTSUITEFLAGS="1098 -d -v" >>> ... >>> ../../tests/dpif-netdev.at:808: >>> ovs-vsctl set interface p1 options:ol_ip_rx_csum_set_good=true >>> ../../tests/dpif-netdev.at:808: >>> ovs-appctl netdev-dummy/receive p1 $(cat good_frame) >>> ../../tests/dpif-netdev.at:808: >>> ovs-pcap p2.pcap > p2.pcap.txt 2>&1 >>> ../../tests/dpif-netdev.at:808: >>> tail -n 1 p2.pcap.txt >>> ../../tests/dpif-netdev.at:808: >>> ovs-vsctl set interface p1 options:ol_ip_rx_csum_set_good=false >>> ... >>> >>> Signed-off-by: David Marchand <[email protected]> >>> Acked-by: Kevin Traynor <[email protected]> >>> --- >>> tests/dpif-netdev.at | 970 ++++++++----------------------------------- >>> 1 file changed, 175 insertions(+), 795 deletions(-) >>> >>> diff --git a/tests/dpif-netdev.at b/tests/dpif-netdev.at >>> index 90d7497664..bbce1ccee4 100644 >>> --- a/tests/dpif-netdev.at >>> +++ b/tests/dpif-netdev.at >>> @@ -736,6 +736,43 @@ AT_CHECK([test `ovs-vsctl get Interface p2 >>> statistics:tx_q0_packets` -gt 0 -a dn >>> OVS_VSWITCHD_STOP >>> AT_CLEANUP >>> >>> +m4_define([CHECK_FWD_PACKET], >>> + [test -z "$3" || AT_CHECK([ovs-vsctl set interface $1 options:$3=true]) >>> + AT_CHECK([ovs-appctl netdev-dummy/receive $1 $(cat $4)]) >>> + test "$5" = 'none' || { >>> + AT_CHECK([ovs-pcap $2.pcap > $2.pcap.txt 2>&1]) >>> + AT_CHECK_UNQUOTED([tail -n 1 $2.pcap.txt], [0], [$(cat $5) >>> +]) >>> + } >>> + test -z "$3" || AT_CHECK([ovs-vsctl remove interface $1 options $3]) >>> +]) >> >> We shouldnt mix the shell and macro checks here. It makes autotest generate >> unnecessary code and makes it harder to userstand which variables are checked >> at runtime and which at compile time. All the arguments here are macro >> arguments that are known at compilation time and so the m4_if macro should >> be used to check them. And we should probbaly use expout instead of the >> unquoted check, i.e.: >> >> diff --git a/tests/dpif-netdev.at b/tests/dpif-netdev.at >> index c41c1e18d..955b0a51c 100644 >> --- a/tests/dpif-netdev.at >> +++ b/tests/dpif-netdev.at >> @@ -737,14 +737,14 @@ OVS_VSWITCHD_STOP >> AT_CLEANUP >> >> m4_define([CHECK_FWD_PACKET], >> - [test -z "$3" || AT_CHECK([ovs-vsctl set interface $1 options:$3=true]) >> + [m4_if([$3], [], [], [AT_CHECK([ovs-vsctl set interface $1 >> options:$3=true])]) >> AT_CHECK([ovs-appctl netdev-dummy/receive $1 $(cat $4)]) >> - test "$5" = 'none' || { >> - AT_CHECK([ovs-pcap $2.pcap > $2.pcap.txt 2>&1]) >> - AT_CHECK_UNQUOTED([tail -n 1 $2.pcap.txt], [0], [$(cat $5) >> -]) >> - } >> - test -z "$3" || AT_CHECK([ovs-vsctl remove interface $1 options $3]) >> + m4_if([$5], [none], [], [ >> + AT_CHECK([ovs-pcap $2.pcap > $2.pcap.txt 2>&1]) >> + AT_CHECK([awk 1 $5 > expout]) dnl Also normalizes the line endings. >> + AT_CHECK([tail -n 1 $2.pcap.txt], [0], [expout]) >> + ]) >> + m4_if([$3], [], [], [AT_CHECK([ovs-vsctl remove interface $1 options >> $3])]) >> ]) >> >> dnl CHECK_IP_CHECKSUMS rx_port tx_port good_pkt bad_pkt good_exp bad_exp > > It is nicer with m4_if yes. > > I don't really see the difference with expout (and there are other > similar constructs that use AT_CHECK_UNQUOTED, in this file). > I don't mind if you update it though. > > >> --- >> >>> + >>> +dnl CHECK_IP_CHECKSUMS rx_port tx_port good_pkt bad_pkt good_exp bad_exp >>> +dnl >>> +dnl Test combinations of Rx IP checksum flags for a good or bad packet >>> +dnl received on rx_port, and sent over tx_port. >>> +m4_define([CHECK_IP_CHECKSUMS], >>> + [dnl Checks for good packet. >>> + dnl No Rx flag >>> + CHECK_FWD_PACKET($1, $2, , $3, $5) >>> + dnl Rx good >>> + CHECK_FWD_PACKET($1, $2, ol_ip_rx_csum_set_good, $3, $5) >>> + dnl Rx bad >>> + CHECK_FWD_PACKET($1, $2, ol_ip_rx_csum_set_bad, $3, $5) >>> + dnl Rx partial >>> + CHECK_FWD_PACKET($1, $2, ol_ip_rx_csum_set_partial, $3, $5) >>> + >>> + dnl Checks for bad packet. >>> + dnl No Rx flag >>> + CHECK_FWD_PACKET($1, $2, , $4, $6) >>> + dnl Rx good >>> + CHECK_FWD_PACKET($1, $2, ol_ip_rx_csum_set_good, $4, $5) >>> + dnl Rx bad >>> + CHECK_FWD_PACKET($1, $2, ol_ip_rx_csum_set_bad, $4, $6) >>> + dnl Rx partial >>> + CHECK_FWD_PACKET($1, $2, ol_ip_rx_csum_set_partial, $4, $5) >>> +]) >>> + >>> AT_SETUP([dpif-netdev - ip csum offload]) >>> AT_KEYWORDS([userspace offload]) >>> OVS_VSWITCHD_START( >>> @@ -747,6 +784,8 @@ OVS_VSWITCHD_START( >>> >>> AT_CHECK([ovs-appctl vlog/set netdev_dummy:file:dbg]) >>> >>> +AT_CHECK([ovs-vsctl set Interface p2 options:tx_pcap=p2.pcap]) >>> + >>> dnl Modify the ip_dst addr to force changing the IP csum. >>> AT_CHECK([ovs-ofctl add-flow br1 >>> in_port=p1,actions=mod_nw_dst:192.168.1.1,output:p2]) >>> >>> @@ -755,160 +794,33 @@ flow_s="\ >>> tcp,ip_src=192.168.123.2,ip_dst=192.168.123.1,ip_frag=no,\ >>> tcp_src=54392,tcp_dst=5201,tcp_flags=ack" >>> >>> -good_frame=$(ovs-ofctl compose-packet --bare "${flow_s}") >>> +ovs-ofctl compose-packet --bare "${flow_s}" > good_frame >> >> It wasn't possible to check the command before, but now it should be >> wrapped into AT_CHECK, so we have a trace of it in the logs and we're >> sure that it doesn't fail silently. >> >>> -bad_frame=$(echo $good_frame | sed -e "s/6b72/dead/") >>> +cat good_frame | sed -e "s/6b72/dead/" > bad_frame >>> dnl 0x6b72 + (5201-2222) == 0x7715 >>> dnl 0xdead + (5201-2222) == 0xea50 >>> -bad_expected=$(echo $good_expected | sed -e "s/7715/ea50/") >>> +cat good_expected | sed -e "s/7715/ea50/" > bad_expected >> These cat / sed commands should also be checked. We can't check all of >> them easily, but we should check ones that we can. >> >> Both checks can be easily added with a simple substitution in vim: >> >> :%s/^ovs-ofctl.*/AT_CHECK([\0])/gc >> :%s/^cat .*/AT_CHECK([\0])/gc > > Ok for me. > >> Since all that mentioned above are just minor test changes, If you agree, >> I can make them on commit. WDYT? > > I don't mind if you do those updates (and the comment on patch 6) yourself. > > > Note that I had some nits from Kevin on the v3 revision, if you could > update them as well, then the series would be good to merge.
Thanks, David, Kevin and Mike! I incorporated all the mentioned changes and applied the set. Also backported patches 1 and 3 down to branch-3.3, and patch 7 to branch-3.6. Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
