| Message ID | 20161209025032.37325-1-diproiettod@vmware.com |
|---|---|
| State | Changes Requested |
| Headers | show |
On Thu, Dec 08, 2016 at 06:50:32PM -0800, Daniele Di Proietto wrote: > The order of the options in the packet generated by ovs-controller > depends on the hash function. I believe that murmur hash (our default) > produces different outputs depending on the endianness of the system. > > This commit fixes the test by reordering the options in the packet > before checking them. > > This was reported before as a failure of the test on x86 with sse42 > enabled, I'm fixing it now because it affects other targets build by > distros by default. > > Reported-at: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=840770 > Signed-off-by: Daniele Di Proietto <diproiettod@vmware.com> I agree that this would solve the problem. I think that it might be nicer to instead sort the options in build_dhcpv4_action() and build_dhcpv6_action() in ovn-northd.c. Then the generated logical actions won't differ from one architecture to another, so that if we one day push a test to a higher level we won't have an underlying source of differences. There's even a helper smap_sort() that could do most of the work. Are you willing to make that change? Thanks, Ben.
On 12/12/2016 13:14, "Ben Pfaff" <blp@ovn.org> wrote: >On Thu, Dec 08, 2016 at 06:50:32PM -0800, Daniele Di Proietto wrote: >> The order of the options in the packet generated by ovs-controller >> depends on the hash function. I believe that murmur hash (our default) >> produces different outputs depending on the endianness of the system. >> >> This commit fixes the test by reordering the options in the packet >> before checking them. >> >> This was reported before as a failure of the test on x86 with sse42 >> enabled, I'm fixing it now because it affects other targets build by >> distros by default. >> >> Reported-at: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=840770 >> Signed-off-by: Daniele Di Proietto <diproiettod@vmware.com> > >I agree that this would solve the problem. > >I think that it might be nicer to instead sort the options in >build_dhcpv4_action() and build_dhcpv6_action() in ovn-northd.c. Then >the generated logical actions won't differ from one architecture to >another, so that if we one day push a test to a higher level we won't >have an underlying source of differences. There's even a helper >smap_sort() that could do most of the work. > >Are you willing to make that change? Sure, it's better than parsing TLVs with awk. I'll send a v2. Thanks, Daniele > >Thanks, > >Ben.
diff --git a/tests/ovn.at b/tests/ovn.at index cb21210..20f0135 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -3751,6 +3751,34 @@ echo "---------------------" echo "------ hv1 dump ----------" as hv1 ovs-ofctl dump-flows br-int +sort_dhcpv4opts_tlv() { + awk ' + function parsehex(h, r,i,d) { + r = 0; + for (i = 1; i <= length(h); i++) { + d = index("0123456789abcdef", substr(h,i,1)); + if (d == 0) { + break; + } + r = r * 16 + (d - 1); + } + return r; + } + + { + opts = $0 + while (length(opts) >= 4) { + type = substr(opts, 1, 2); + s_len = substr(opts, 3, 2); + len = parsehex(s_len) + value = substr(opts, 5, len * 2); + opts = substr(opts, 5 + len * 2); + print type s_len value; + } + print opts + }' | sort | tr -d "\n\r" +} + # Send DHCPDISCOVER. offer_ip=`ip_to_hex 10 0 0 4` server_ip=`ip_to_hex 10 0 0 1` @@ -3764,8 +3792,13 @@ $PYTHON "$top_srcdir/utilities/ovs-pcap.in" hv1/vif1-tx.pcap > 1.packets cat 1.expected | cut -c -48 > expout AT_CHECK([cat 1.packets | cut -c -48], [0], [expout]) # Skipping the IPv4 checksum. -cat 1.expected | cut -c 53- > expout -AT_CHECK([cat 1.packets | cut -c 53-], [0], [expout]) +cat 1.expected | cut -c 53-570 > expout +AT_CHECK([cat 1.packets | cut -c 53-570], [0], [expout]) +# The ordering of the options in the packet depends on the hash function used +# and possibly on the endianness of the system. We parse and sort both the +# expected and the generated packet. +cat 1.expected | cut -c 571 | sort_dhcpv4opts_tlv > expout +AT_CHECK([cat 1.packets | cut -c 571 | sort_dhcpv4opts_tlv], [0], [expout]) # ovs-ofctl also resumes the packets and this causes other ports to receive # the DHCP request packet. So reset the pcap files so that its easier to test. @@ -3787,8 +3820,13 @@ $PYTHON "$top_srcdir/utilities/ovs-pcap.in" hv1/vif2-tx.pcap > 2.packets cat 2.expected | cut -c -48 > expout AT_CHECK([cat 2.packets | cut -c -48], [0], [expout]) # Skipping the IPv4 checksum. -cat 2.expected | cut -c 53- > expout -AT_CHECK([cat 2.packets | cut -c 53-], [0], [expout]) +cat 2.expected | cut -c 53-570 > expout +AT_CHECK([cat 2.packets | cut -c 53-570], [0], [expout]) +# The ordering of the options in the packet depends on the hash function used +# and possibly on the endianness of the system. We parse and sort both the +# expected and the generated packet. +cat 2.expected | cut -c 571 | sort_dhcpv4opts_tlv > expout +AT_CHECK([cat 2.packets | cut -c 571 | sort_dhcpv4opts_tlv], [0], [expout]) reset_pcap_file hv1-vif1 hv1/vif1 reset_pcap_file hv1-vif2 hv1/vif2
The order of the options in the packet generated by ovs-controller depends on the hash function. I believe that murmur hash (our default) produces different outputs depending on the endianness of the system. This commit fixes the test by reordering the options in the packet before checking them. This was reported before as a failure of the test on x86 with sse42 enabled, I'm fixing it now because it affects other targets build by distros by default. Reported-at: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=840770 Signed-off-by: Daniele Di Proietto <diproiettod@vmware.com> --- tests/ovn.at | 46 ++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 42 insertions(+), 4 deletions(-)