Thanks Yi-hung One minor comment inline.
On Sat, Mar 10, 2018 at 9:30 AM, Yi-Hung Wei <[email protected]> wrote: > Currently, using ofproto/trace to trace a datapath flow with eth_type() > but without eth() may hit unexpected behavior because OVS sets > the packet_type to be (1, eth_type) when decoding the odp flow key. > This patch updates the logic of odp flow key decoding so that the > packet_type defaults to (0,0) unless user explicitly specifies the > packet_type. An unit test is added to prevent future regression. > > Travis build: https://travis-ci.org/YiHungWei/ovs/builds/351565383 > > Reported-by: Amar Padmanabhan <[email protected]> > Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2017-Dece > mber/045817.html > Reported-by: Su Wang <[email protected]> > VMWare-BZ: #2070488 > Signed-off-by: Yi-Hung Wei <[email protected]> > --- > lib/odp-util.c | 8 -------- > tests/ofproto-dpif.at | 24 ++++++++++++++++++++++++ > 2 files changed, 24 insertions(+), 8 deletions(-) > > diff --git a/lib/odp-util.c b/lib/odp-util.c > index 5da83b4c64c4..682f1d3bd4b5 100644 > --- a/lib/odp-util.c > +++ b/lib/odp-util.c > @@ -6440,14 +6440,6 @@ odp_flow_key_to_flow__(const struct nlattr *key, > size_t key_len, > } > expected_attrs |= UINT64_C(1) << OVS_KEY_ATTR_ETHERNET; > } > - else if (present_attrs & (UINT64_C(1) << OVS_KEY_ATTR_ETHERTYPE)) { > - ovs_be16 ethertype = nl_attr_get_be16(attrs[OVS_KEY > _ATTR_ETHERTYPE]); > - if (!is_mask) { > - flow->packet_type = PACKET_TYPE_BE(OFPHTN_ETHERTYPE, > - ntohs(ethertype)); > - } > - expected_attrs |= UINT64_C(1) << OVS_KEY_ATTR_ETHERTYPE; > - } > > /* Get Ethertype or 802.1Q TPID or FLOW_DL_TYPE_NONE. */ > if (!parse_ethertype(attrs, present_attrs, &expected_attrs, flow, > diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at > index d2058eddd3eb..13e57b90edd1 100644 > --- a/tests/ofproto-dpif.at > +++ b/tests/ofproto-dpif.at > @@ -10162,3 +10162,27 @@ AT_CHECK([grep flow_del ovs-vswitchd.log], [1]) > > OVS_VSWITCHD_STOP > AT_CLEANUP > + > +AT_SETUP([ofproto-dpif - debug ofproto/trace]) > +OVS_VSWITCHD_START > +add_of_ports br0 1 2 3 4 > +AT_DATA([flows.txt], [dnl > +table=0 priority=100 in_port=1,udp actions=output:2 > +table=0 priority=99 in_port=1 actions=output:3 > +]) > +AT_CHECK([ovs-ofctl -O OpenFlow15 add-flows br0 flows.txt]) > + > +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy > 'in_port(1),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.16 > 8.0.2,proto=17,tos=0,ttl=128,frag=no),udp(src=8,dst=9)'], [0], [stdout]) > +AT_CHECK([tail -1 stdout], [0], > + [Datapath actions: 2 > +]) > + > +AT_CHECK([ovs-ofctl -O OpenFlow15 add-flow br0 "table=0 priority=100 > in_port=1,packet_type=(1,0x800) actions=output:4"]) > + > +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy > 'in_port(1),packet_type(ns=1,id=0x800),eth_type(0x0800),ipv4 > (src=192.168.0.1,dst=192.168.0.2,proto=17,tos=0,ttl=128,frag=no),udp(src=8,dst=9)'], > [0], [stdout]) > +AT_CHECK([cat stdout | grep output], [0], > + [ output:4 > +]) > + > +OVS_VSWITCHD_STOP > +AT_CLEANUP > Would the test be more convincing and stronger, if all rules are present before the first packet test ? It is not required to check this specific issue, though. Below is a short way to express this; otherwise LGTM. AT_SETUP([ofproto-dpif - debug ofproto/trace]) OVS_VSWITCHD_START add_of_ports br0 1 2 3 4 AT_DATA([flows.txt], [dnl table=0 priority=100 in_port=1,udp actions=output:2 table=0 priority=100 in_port=1,packet_type=(1,0x800),actions=output:4 table=0 priority=99 in_port=1 actions=output:3 ]) AT_CHECK([ovs-ofctl -O OpenFlow15 add-flows br0 flows.txt]) AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'in_port(1),eth_type(0x0800),i pv4(src=192.168.0.1,dst=192.168.0.2,proto=17,tos=0,ttl=128,frag=no),udp(src=8,dst=9)'], [0], [stdout]) AT_CHECK([tail -1 stdout], [0], [Datapath actions: 2 ]) AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'in_port(1),packet_type(ns=1,i d=0x800),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0 .2,proto=17,tos=0,ttl=128,frag=no),udp(src=8,dst=9)'], [0], [stdout]) AT_CHECK([cat stdout | grep output], [0], [ output:4 ]) > -- > 2.7.4 > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
