On 13/03/2023 9:16, Ales Musil wrote:
> Specifying datapath with "dpctl/flush-conntrack" didn't
> work as expected and caused error:
> ovs-dpctl: field system@ovs-system missing value (Invalid argument)
>
> To prevent that, check if we have datapath as first argument
> and use it accordingly.
>
> Also add couple of test cases to ensure that everything works as
> expocted.
>
> Fixes: a9ae73b916ba ("ofp, dpif: Allow CT flush based on partial match.")
> Signed-off-by: Ales Musil <[email protected]>
> ---
> v2: Add test case.
> v3: Add test case also for flush-conntrack without arguments.
> v4: Rebase on top of current master.
> Add negative test cases for the cli utilities.
> Fix the report of unparsed arguments for both dpctl and ofctl.
> ---
> lib/dpctl.c | 12 +++++++++---
> tests/system-traffic.at | 42 +++++++++++++++++++++++++++++++++++++++++
> utilities/ovs-ofctl.c | 4 ++++
> 3 files changed, 55 insertions(+), 3 deletions(-)
>
> diff --git a/lib/dpctl.c b/lib/dpctl.c
> index c501a0cd7..59cc4f58c 100644
> --- a/lib/dpctl.c
> +++ b/lib/dpctl.c
> @@ -1717,10 +1717,16 @@ dpctl_flush_conntrack(int argc, const char *argv[],
> uint16_t zone, *pzone = NULL;
> int error;
> int args = argc - 1;
> + int zone_pos = 1;
> +
> + if (dp_arg_exists(argc, argv)) {
> + args--;
> + zone_pos = 2;
> + }
>
> /* Parse zone. */
> - if (args && !strncmp(argv[1], "zone=", 5)) {
> - if (!ovs_scan(argv[1], "zone=%"SCNu16, &zone)) {
> + if (args && !strncmp(argv[zone_pos], "zone=", 5)) {
> + if (!ovs_scan(argv[zone_pos], "zone=%"SCNu16, &zone)) {
> ds_put_cstr(&ds, "failed to parse zone");
> error = EINVAL;
> goto error;
> @@ -1748,7 +1754,7 @@ dpctl_flush_conntrack(int argc, const char *argv[],
> }
>
> /* Report error if there is more than one unparsed argument. */
> - if (args > 1) {
> + if (args > 0) {
> ds_put_cstr(&ds, "invalid arguments");
> error = EINVAL;
> goto error;
> diff --git a/tests/system-traffic.at b/tests/system-traffic.at
> index 380372430..464e042a9 100644
> --- a/tests/system-traffic.at
> +++ b/tests/system-traffic.at
> @@ -2360,8 +2360,10 @@ priority=100,in_port=2,icmp,action=ct(zone=5,commit),1
> ])
>
> AT_CHECK([ovs-ofctl --bundle add-flows br0 flows.txt])
> +dp=$(ovs-appctl dpctl/dump-dps)
>
> m4_foreach([FLUSH_CMD], [[ovs-appctl dpctl/flush-conntrack],
> + [ovs-appctl dpctl/flush-conntrack $dp],
> [ovs-ofctl ct-flush br0]], [
> AS_BOX([Testing with FLUSH_CMD])
>
> @@ -2504,8 +2506,48 @@
> udp,orig=(src=10.1.1.2,dst=10.1.1.1,sport=2,dport=1),reply=(src=10.1.1.1,dst=10.
> AT_CHECK([FLUSH_CMD zone=5 '' 'ct_nw_src=10.1.1.1'])
>
> AT_CHECK([ovs-appctl dpctl/dump-conntrack | grep "10\.1\.1\.1"], [1])
> +
> +dnl Test UDP from port 1 and 2, flush without arguments
> +AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 "in_port=1
> packet=50540000000a50540000000908004500001c000000000011a4cd0a0101010a0101020001000200080000
> actions=resubmit(,0)"])
> +AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 "in_port=2
> packet=50540000000a50540000000908004500001c000000000011a4cd0a0101020a0101010002000100080000
> actions=resubmit(,0)"])
> +
> +
> +AT_CHECK([ovs-appctl dpctl/dump-conntrack | grep "10\.1\.1\.1" | sort], [0],
> [dnl
> +udp,orig=(src=10.1.1.1,dst=10.1.1.2,sport=1,dport=2),reply=(src=10.1.1.2,dst=10.1.1.1,sport=2,dport=1)
> +udp,orig=(src=10.1.1.2,dst=10.1.1.1,sport=2,dport=1),reply=(src=10.1.1.1,dst=10.1.1.2,sport=1,dport=2),zone=5
> ])
>
> +AT_CHECK([FLUSH_CMD])
> +
> +AT_CHECK([ovs-appctl dpctl/dump-conntrack | grep "10\.1\.1\.1"], [1])
> +])
> +
> +dnl Test flush with invalid arguments
> +
> +AT_CHECK([ovs-appctl dpctl/flush-conntrack zone=invalid 'ct_nw_src=10.1.1.1'
> 'ct_nw_dst=10.1.1.1'], [2], [ignore], [stderr])
> +AT_CHECK([grep -q "failed to parse zone" stderr])
> +
> +AT_CHECK([ovs-appctl dpctl/flush-conntrack zone=1
> 'ct_nw_src=10.1.1.1,invalid=invalid' 'ct_nw_dst=10.1.1.1'], [2], [ignore],
> [stderr])
> +AT_CHECK([grep -q "invalid conntrack tuple field: invalid" stderr])
> +
> +AT_CHECK([ovs-appctl dpctl/flush-conntrack zone=1 'ct_nw_src=invalid'
> 'ct_nw_dst=10.1.1.1'], [2], [ignore], [stderr])
> +AT_CHECK([grep -q "failed to parse field ct_nw_src" stderr])
> +
> +AT_CHECK([ovs-appctl dpctl/flush-conntrack zone=1 'ct_nw_src=10.1.1.1'
> 'ct_nw_dst=10.1.1.1' invalid], [2], [ignore], [stderr])
> +AT_CHECK([grep -q "invalid arguments" stderr])
> +
> +AT_CHECK([ovs-appctl dpctl/flush-conntrack $dp zone=1 'ct_nw_src=10.1.1.1'
> 'ct_nw_dst=10.1.1.1' invalid], [2], [ignore], [stderr])
> +AT_CHECK([grep -q "command takes at most 4 arguments" stderr])
> +
> +AT_CHECK([ovs-appctl dpctl/flush-conntrack $dp 'ct_nw_src=10.1.1.1'
> 'ct_nw_dst=10.1.1.1' invalid], [2], [ignore], [stderr])
> +AT_CHECK([grep -q "invalid arguments" stderr])
> +
> +AT_CHECK([ovs-ofctl ct-flush br0 zone=1 'ct_nw_src=10.1.1.1'
> 'ct_nw_dst=10.1.1.1' invalid], [1], [ignore], [stderr])
> +AT_CHECK([grep -q "command takes at most 4 arguments" stderr])
> +
> +AT_CHECK([ovs-ofctl ct-flush br0 'ct_nw_src=10.1.1.1' 'ct_nw_dst=10.1.1.1'
> invalid], [1], [ignore], [stderr])
> +AT_CHECK([grep -q "Invalid arguments" stderr])
> +
> OVS_TRAFFIC_VSWITCHD_STOP
> AT_CLEANUP
>
> diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
> index eabec18a3..3ce4e82ec 100644
> --- a/utilities/ovs-ofctl.c
> +++ b/utilities/ovs-ofctl.c
> @@ -3089,6 +3089,10 @@ ofctl_ct_flush(struct ovs_cmdl_context *ctx)
> args--;
> }
>
> + if (args > 0) {
> + ovs_fatal(0, "Invalid arguments");
> + }
> +
> open_vconn(ctx->argv[1], &vconn);
> enum ofp_version version = vconn_get_version(vconn);
> struct ofpbuf *msg = ofp_ct_match_encode(&match, pzone, version);
thanks
Reviewed-by: Roi Dayan <[email protected]>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
