Thanks Yi-hung
One minor comment inline.

On Sat, Mar 10, 2018 at 9:30 AM, Yi-Hung Wei <yihung....@gmail.com> 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 <amarpadmanab...@fb.com>
> Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2017-Dece
> mber/045817.html
> Reported-by: Su Wang <suw...@vmware.com>
> VMWare-BZ: #2070488
> Signed-off-by: Yi-Hung Wei <yihung....@gmail.com>
> ---
>  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
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to