Some small remarks.

On 22 Nov 2021, at 12:22, Adrian Moreno wrote:

> test-odp is used to parse datapath flow actions and matches within the
> odp tests. Wrap calls to this tool in a python script that also parses
> them using the python flow parsing library.
>
> Signed-off-by: Adrian Moreno <[email protected]>
> ---
>  tests/automake.mk         |  3 +-
>  tests/odp.at              | 36 ++++++++---------
>  tests/ovs-test-dpparse.py | 83 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 103 insertions(+), 19 deletions(-)
>  create mode 100755 tests/ovs-test-dpparse.py
>
> diff --git a/tests/automake.mk b/tests/automake.mk
> index 5dc805ef4..be69f3d16 100644
> --- a/tests/automake.mk
> +++ b/tests/automake.mk
> @@ -21,7 +21,8 @@ EXTRA_DIST += \
>       $(srcdir)/package.m4 \
>       $(srcdir)/tests/testsuite \
>       $(srcdir)/tests/testsuite.patch \
> -     $(srcdir)/tests/ovs-test-ofparse.py
> +     $(srcdir)/tests/ovs-test-ofparse.py \
> +     $(srcdir)/tests/ovs-test-dpparse.py

Add in alphabetical order

And maybe also add it to CHECK_PYFILES to capture flake8 errors.

>
>  COMMON_MACROS_AT = \
> diff --git a/tests/odp.at b/tests/odp.at
> index 07a5cfe39..69e86c5c1 100644
> --- a/tests/odp.at
> +++ b/tests/odp.at
> @@ -105,7 +105,7 @@ sed -i'back' 
> 's/\(skb_mark(0)\),\(ct\)/\1,ct_state(0),ct_zone(0),\2/' odp-out.tx
>  sed -i'back' 
> 's/\(skb_mark([[^)]]*)\),\(recirc\)/\1,ct_state(0),ct_zone(0),ct_mark(0),ct_label(0),\2/'
>  odp-out.txt
>  sed -i'back' 's/\(in_port(1)\),\(eth\)/\1,packet_type(ns=0,id=0),\2/' 
> odp-out.txt
>
> -AT_CHECK_UNQUOTED([ovstest test-odp parse-keys < odp-in.txt], [0], [`cat 
> odp-out.txt`
> +AT_CHECK_UNQUOTED([ovs-test-dpparse.py ovstest test-odp parse-keys < 
> odp-in.txt], [0], [`cat odp-out.txt`
>  ])
>  AT_CLEANUP
>
> @@ -192,7 +192,7 @@ sed -n 's/,frag=no),.*/,frag=later)/p' odp-base.txt
>   sed 
> 's/^/skb_priority(0),tunnel(tun_id=0xfedcba9876543210,src=10.0.0.1,dst=10.0.0.2,ttl=128,erspan(ver=2,dir=1,hwid=0x7/0xf),flags(df|key)),skb_mark(0),recirc_id(0),dp_hash(0),/'
>  odp-base.txt
>  ) > odp.txt
>  AT_CAPTURE_FILE([odp.txt])
> -AT_CHECK_UNQUOTED([ovstest test-odp parse-wc-keys < odp.txt], [0], [`cat 
> odp.txt`
> +AT_CHECK_UNQUOTED([ovs-test-dpparse.py ovstest test-odp parse-wc-keys < 
> odp.txt], [0], [`cat odp.txt`
>  ])
>  AT_CLEANUP
>
> @@ -239,25 +239,25 @@ AT_DATA([odp-tcp6.txt], [dnl
>  
> in_port(1),eth(src=00:01:02:03:04:05,dst=10:11:12:13:14:15),eth_type(0x86dd),ipv6(src=::1/::255,dst=::2/::255,label=0/0xf0,proto=10/0xf0,tclass=0x70/0xf0,hlimit=128/0xf0,frag=no)
>  
> in_port(1),eth(src=00:01:02:03:04:05,dst=10:11:12:13:14:15),eth_type(0x86dd),ipv6(src=::1,dst=::2,label=0,proto=6,tclass=0,hlimit=128,frag=no),tcp(src=80/0xff00,dst=8080/0xff)
>  ])
> -AT_CHECK_UNQUOTED([ovstest test-odp parse-filter filter='dl_type=0x1235' < 
> odp-base.txt], [0], [`cat odp-eth-type.txt`
> +AT_CHECK_UNQUOTED([ovs-test-dpparse.py ovstest test-odp parse-filter 
> filter='dl_type=0x1235' < odp-base.txt], [0], [`cat odp-eth-type.txt`
>  ])
> -AT_CHECK_UNQUOTED([ovstest test-odp parse-filter filter='dl_vlan=99' < 
> odp-vlan-base.txt], [0], [`cat odp-vlan.txt`
> +AT_CHECK_UNQUOTED([ovs-test-dpparse.py ovstest test-odp parse-filter 
> filter='dl_vlan=99' < odp-vlan-base.txt], [0], [`cat odp-vlan.txt`
>  ])
> -AT_CHECK_UNQUOTED([ovstest test-odp parse-filter filter='dl_vlan=99,ip' < 
> odp-vlan-base.txt], [0], [`cat odp-vlan.txt`
> +AT_CHECK_UNQUOTED([ovs-test-dpparse.py ovstest test-odp parse-filter 
> filter='dl_vlan=99,ip' < odp-vlan-base.txt], [0], [`cat odp-vlan.txt`
>  ])
> -AT_CHECK_UNQUOTED([ovstest test-odp parse-filter 
> filter='ip,nw_src=35.8.2.199' < odp-base.txt], [0], [`cat odp-ipv4.txt`
> +AT_CHECK_UNQUOTED([ovs-test-dpparse.py ovstest test-odp parse-filter 
> filter='ip,nw_src=35.8.2.199' < odp-base.txt], [0], [`cat odp-ipv4.txt`
>  ])
> -AT_CHECK_UNQUOTED([ovstest test-odp parse-filter 
> filter='ip,nw_dst=172.16.0.199' < odp-base.txt], [0], [`cat odp-ipv4.txt`
> +AT_CHECK_UNQUOTED([ovs-test-dpparse.py ovstest test-odp parse-filter 
> filter='ip,nw_dst=172.16.0.199' < odp-base.txt], [0], [`cat odp-ipv4.txt`
>  ])
> -AT_CHECK_UNQUOTED([ovstest test-odp parse-filter 
> filter='dl_type=0x0800,nw_src=35.8.2.199,nw_dst=172.16.0.199' < 
> odp-base.txt], [0], [`cat odp-ipv4.txt`
> +AT_CHECK_UNQUOTED([ovs-test-dpparse.py ovstest test-odp parse-filter 
> filter='dl_type=0x0800,nw_src=35.8.2.199,nw_dst=172.16.0.199' < 
> odp-base.txt], [0], [`cat odp-ipv4.txt`
>  ])
> -AT_CHECK_UNQUOTED([ovstest test-odp parse-filter 
> filter='icmp,nw_src=35.8.2.199' < odp-base.txt], [0], [`cat odp-icmp.txt`
> +AT_CHECK_UNQUOTED([ovs-test-dpparse.py ovstest test-odp parse-filter 
> filter='icmp,nw_src=35.8.2.199' < odp-base.txt], [0], [`cat odp-icmp.txt`
>  ])
> -AT_CHECK_UNQUOTED([ovstest test-odp parse-filter 
> filter='arp,arp_spa=1.2.3.5' < odp-base.txt], [0], [`cat odp-arp.txt`
> +AT_CHECK_UNQUOTED([ovs-test-dpparse.py ovstest test-odp parse-filter 
> filter='arp,arp_spa=1.2.3.5' < odp-base.txt], [0], [`cat odp-arp.txt`
>  ])
> -AT_CHECK_UNQUOTED([ovstest test-odp parse-filter filter='tcp,tp_src=90' < 
> odp-base.txt], [0], [`cat odp-tcp.txt`
> +AT_CHECK_UNQUOTED([ovs-test-dpparse.py ovstest test-odp parse-filter 
> filter='tcp,tp_src=90' < odp-base.txt], [0], [`cat odp-tcp.txt`
>  ])
> -AT_CHECK_UNQUOTED([ovstest test-odp parse-filter filter='tcp6,tp_src=90' < 
> odp-base.txt], [0], [`cat odp-tcp6.txt`
> +AT_CHECK_UNQUOTED([ovs-test-dpparse.py ovstest test-odp parse-filter 
> filter='tcp6,tp_src=90' < odp-base.txt], [0], [`cat odp-tcp6.txt`
>  ])
>  AT_CLEANUP
>
> @@ -385,14 +385,14 @@ check_pkt_len(size=200,gt(ct(nat)),le(drop))
>  
> check_pkt_len(size=200,gt(set(eth(src=00:01:02:03:04:05,dst=10:11:12:13:14:15))),le(set(eth(src=00:01:02:03:04:06,dst=10:11:12:13:14:16))))
>  lb_output(1)
>  ])
> -AT_CHECK_UNQUOTED([ovstest test-odp parse-actions < actions.txt], [0],
> +AT_CHECK_UNQUOTED([ovs-test-dpparse.py ovstest test-odp parse-actions < 
> actions.txt], [0],
>    [`cat actions.txt`
>  ])
>  AT_CLEANUP
>
>  AT_SETUP([OVS datapath actions parsing and formatting - invalid forms])
>  dnl This caused a hang in older versions.
> -AT_CHECK([echo 'encap_nsh@:{@' | ovstest test-odp parse-actions
> +AT_CHECK([echo 'encap_nsh@:{@' | ovs-test-dpparse.py ovstest test-odp 
> parse-actions
>  ], [0], [dnl
>  odp_actions_from_string: error
>  ])
> @@ -427,7 +427,7 @@ data_invalid=$(printf '%*s' 131018 | tr ' ' "a")
>  echo "userspace(pid=1234567,userdata(${data_valid}),tunnel_out_port=10)" >> 
> actions.txt
>  echo "userspace(pid=1234567,userdata(${data_invalid}),tunnel_out_port=10)" 
> >> actions.txt
>
> -AT_CHECK_UNQUOTED([ovstest test-odp parse-actions < actions.txt], [0], [dnl
> +AT_CHECK_UNQUOTED([ovs-test-dpparse.py ovstest test-odp parse-actions < 
> actions.txt], [0], [dnl
>  `cat actions.txt | head -1`
>  odp_actions_from_string: error
>  `cat actions.txt | head -3 | tail -1`
> @@ -443,7 +443,7 @@ actions=$(printf 'set(encap()),%.0s' $(seq 8190))
>  echo "${actions}set(encap())" > actions.txt
>  echo "${actions}set(encap()),set(encap())" >> actions.txt
>
> -AT_CHECK_UNQUOTED([ovstest test-odp parse-actions < actions.txt], [0], [dnl
> +AT_CHECK_UNQUOTED([ovs-test-dpparse.py ovstest test-odp parse-actions < 
> actions.txt], [0], [dnl
>  `cat actions.txt | head -1`
>  odp_actions_from_string: error
>  ])
> @@ -457,7 +457,7 @@ dnl sequence of keys.  'syntax error' indicates oversized 
> list of keys.
>  keys=$(printf 'encap(),%.0s' $(seq 16382))
>  echo "${keys}encap()" > keys.txt
>  echo "${keys}encap(),encap()" >> keys.txt
> -AT_CHECK([ovstest test-odp parse-keys < keys.txt | sed 's/encap(),//g'], 
> [0], [dnl
> +AT_CHECK([ovs-test-dpparse.py ovstest test-odp parse-keys < keys.txt | sed 
> 's/encap(),//g'], [0], [dnl
>  odp_flow_key_to_flow: error (duplicate encap attribute in flow key; the flow 
> key in error is: encap())
>  odp_flow_from_string: error (syntax error at encap())
>  ])
> @@ -467,7 +467,7 @@ AT_SETUP([OVS datapath keys parsing and formatting - 33 
> nested encap ])
>  AT_DATA([odp-in.txt], [dnl
>  
> encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap()))))))))))))))))))))))))))))))))
>  ])
> -AT_CHECK_UNQUOTED([ovstest test-odp parse-keys < odp-in.txt], [0], [dnl
> +AT_CHECK_UNQUOTED([ovs-test-dpparse.py ovstest test-odp parse-keys < 
> odp-in.txt], [0], [dnl
>  odp_flow_from_string: error (syntax error at 
> encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap())))))))))))))))))))))))))))))))))
>  ])
>  AT_CLEANUP
> diff --git a/tests/ovs-test-dpparse.py b/tests/ovs-test-dpparse.py
> new file mode 100755
> index 000000000..455b6ea22
> --- /dev/null
> +++ b/tests/ovs-test-dpparse.py
> @@ -0,0 +1,83 @@
> +#!/usr/bin/env python3
> +# Copyright (c) 2021 Red Hat, Inc.
> +#
> +# Licensed under the Apache License, Version 2.0 (the "License");
> +# you may not use this file except in compliance with the License.
> +# You may obtain a copy of the License at:
> +#
> +#     http://www.apache.org/licenses/LICENSE-2.0
> +#
> +# Unless required by applicable law or agreed to in writing, software
> +# distributed under the License is distributed on an "AS IS" BASIS,
> +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> +# See the License for the specific language governing permissions and
> +# limitations under the License.
> +
> +# Breaks lines read from stdin into groups using blank lines as
> +# group separators, then sorts lines within the groups for
> +# reproducibility.

Left over?

> +# ovs-test-ofparse is just a wrapper around ovs-ofctl

Cut/paste without updating? ovs-test-dpparse and ovstest test-odp

> +# that also runs the python flow parsing utility to check that flows are
> +# parseable

Add dot.

> +
> +import subprocess
> +import sys
> +import re
> +
> +from ovs.flows.odp import ODPFlowFactory
> +
> +diff_regexp = re.compile(r"\d{2}: (\d{2}|\(none\)) -> (\d{2}|\(none\))$")
> +
> +
> +def run(input_data):
> +    p = subprocess.Popen(
> +        sys.argv[1:],
> +        stdin=subprocess.PIPE,
> +        stdout=subprocess.PIPE,
> +        stderr=subprocess.PIPE,
> +    )
> +    out, err = p.communicate(input_data.encode("utf-8"))
> +
> +    print(out.decode("utf-8"), file=sys.stdout, end="")
> +    print(err.decode("utf-8"), file=sys.stderr, end="")
> +    return p.returncode, out, err
> +
> +
> +def main():
> +    return_code = 0
> +    input_data = sys.stdin.read()
> +    return_code, out, err = run(input_data)
> +
> +    if return_code == 0:
> +        flows = list()
> +        for line in input_data.split("\n"):
> +            if not (
> +                "error" in line  # skip errors
> +                or line.strip() == ""  # skip empty lines
> +                or line.strip()[0] == "#"  # skip comments
> +            ):
> +                flows.append(line)
> +
> +        factory = ODPFlowFactory()
> +        for flow in flows:
> +            if any(
> +                c in sys.argv
> +                for c in ["parse-keys", "parse-wc-keys", "parse-filter"]
> +            ):
> +                # Add actions=drop so that the flow is properly formatted
> +                flow += " actions:drop"
> +            elif "parse-actions" in sys.argv:
> +                flow = "actions:" + flow

I’m wondering why “malformed odp flow: %s" % odp_string” is not generated as 
you are missing the matching part?
Guess it’s getting late already ;)

> +            try:
> +                _ = factory.from_string(flow)

Should the _ here not be the same as flow? Guess checking this should be a good 
test to see if the conversion is good both ways?
I hacked this for this patch, and it seem to work:

            try:
                result_flow = factory.from_string(flow)
                if flow != str(result_flow):
                    print("in : {}".format(flow))
                    print("out: {}".format(result_flow))
                    raise ValueError("Flow conversion back to string failed!")

Same for the previous patch?

> +            except Exception as e:
> +                print(e)
> +                return 1
> +
> +    return return_code
> +
> +
> +if __name__ == "__main__":
> +    sys.exit(main())
> -- 
> 2.31.1

_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to