| Message ID | 20230313071635.89349-1-amusil@redhat.com |
|---|---|
| State | Accepted |
| Commit | ebe98c587deb1bc011d9ec6263dc25bed9df0d3b |
| Headers | show |
| Series | [ovs-dev,v4,1/2] dpctl: Fix flush-conntrack with datapath as argument | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
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 <amusil@redhat.com> > --- > 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 <roid@nvidia.com>
On Mon, Mar 13, 2023 at 08:16:34AM +0100, 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 <amusil@redhat.com> Reviewed-by: Simon Horman <simon.horman@corigine.com>
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);
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 <amusil@redhat.com> --- 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(-)