On 7 October 2015 at 11:11, Daniele Di Proietto <[email protected]> wrote: > Hi Joe, > > I have a couple of minor comments inline > > On 02/10/2015 22:16, "Joe Stringer" <[email protected]> wrote: > >>This patch adds a new action and fields to OVS that allow connection >>tracking to be performed. This support works in conjunction with the >>Linux kernel support merged into the Linux-4.3 development cycle. > [...] >>diff --git a/lib/odp-util.c b/lib/odp-util.c >>index c173623..a570afa 100644 >>--- a/lib/odp-util.c >>+++ b/lib/odp-util.c > [...] >>@@ -2605,6 +2790,25 @@ scan_tcp_flags(const char *s, ovs_be16 *key, >>ovs_be16 *mask) >> } >> >> static int >>+scan_ct_state(const char *s, uint32_t *key, uint32_t *mask) >>+{ >>+ uint32_t flags, fmask; >>+ int n; >>+ >>+ n = parse_flags(s, ct_state_to_string, ')', NULL, NULL, &flags, >>+ CS_SUPPORTED_MASK, mask ? &fmask : NULL); >>+ > > Should we scan these flags as OVS_CS_F* (the odp format)? > Here they're treated as CS_* (the OpenFlow format). > > In this case it could be something like > > n = parse_flags(s, odp_ct_state_to_string, ')', NULL, NULL, &flags, > ovs_to_odp_ct_state(CS_SUPPORTED_MASK, false), > mask ? &fmask : NULL); > > > >>+ if (n >= 0) { >>+ *key = flags; >>+ if (mask) { >>+ *mask = fmask; >>+ } >>+ return n; >>+ } >>+ return 0; >>+} >>+ >>+static int >> scan_frag(const char *s, uint8_t *key, uint8_t *mask) >> { >> int n; > [...] >> >> >>diff --git a/tests/odp.at b/tests/odp.at >>index 61edde3..16d7411 100644 >>--- a/tests/odp.at >>+++ b/tests/odp.at >>@@ -86,6 +86,11 @@ sed '/bos=0/{ >> s/^/ODP_FIT_TOO_LITTLE: / >> }' < odp-in.txt > odp-out.txt >> >>+dnl Some fields are always printed for this test, because wildcards >>aren't >>+dnl specified. We can skip these. >>+sed -i 's/\(skb_mark(0)\),\(ct\)/\1,ct_state(0),ct_zone(0),\2/' >>odp-out.txt >>+sed -i >>'s/\(skb_mark([[^)]]*)\),\(recirc\)/\1,ct_state(0),ct_zone(0),\2/' >>odp-out.txt >>+ >> AT_CHECK_UNQUOTED([ovstest test-odp parse-keys < odp-in.txt], [0], [`cat >>odp-out.txt` >> ]) >> AT_CLEANUP >>@@ -153,6 +158,10 @@ >>s/\(eth([[^)]]*)\),*/\1,eth_type(0x8100),vlan(vid=99,pcp=7),encap(/ >> s/$/)/' odp-base.txt >> >> echo >>+ echo '# Valid forms with conntrack fields.' >>+ sed 's/\(eth([[^)]]*)\),/\1,ct_state(+trk),ct_zone(5/0xff),/' >>odp-base.txt > > I think we should escape the forward slash and use a hex prefix for the > zone > > ...ct_zone(0x5\/0xff)... > > > With this fix, the testcase should detect the above issue.
Thanks again, those fixes look right to me. I'll roll them in. _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
