| Message ID | 20230306133706.156615-1-amusil@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev,v2,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 | fail | github build: failed |
| ovsrobot/intel-ovs-compilation | success | test: success |
On 3/6/23 14:37, 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. > > Signed-off-by: Ales Musil <amusil@redhat.com> This needs a Fixes tag. > --- > v2: Add test case. > --- > lib/dpctl.c | 17 ++++++++--------- > tests/system-traffic.at | 2 ++ > 2 files changed, 10 insertions(+), 9 deletions(-) > > diff --git a/lib/dpctl.c b/lib/dpctl.c > index c501a0cd7..a7a4d8ee8 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; > @@ -1747,13 +1753,6 @@ dpctl_flush_conntrack(int argc, const char *argv[], > args--; > } > > - /* Report error if there is more than one unparsed argument. */ > - if (args > 1) { > - ds_put_cstr(&ds, "invalid arguments"); > - error = EINVAL; > - goto error; > - } > - > error = opt_dpif_open(argc, argv, dpctl_p, 5, &dpif); > if (error) { > dpctl_error(dpctl_p, error, "Cannot open dpif"); > diff --git a/tests/system-traffic.at b/tests/system-traffic.at > index 380372430..c6ec61dba 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]) > It looks like we don't have tests for a case where FLUSH_CMD is called without any arguments while all of them are optional. Could you, please, add one? Best regards, Ilya Maximets.
On Thu, Mar 9, 2023 at 1:11 PM Ilya Maximets <i.maximets@ovn.org> wrote: > On 3/6/23 14:37, 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. > > > > Signed-off-by: Ales Musil <amusil@redhat.com> > > This needs a Fixes tag. > > > --- > > v2: Add test case. > > --- > > lib/dpctl.c | 17 ++++++++--------- > > tests/system-traffic.at | 2 ++ > > 2 files changed, 10 insertions(+), 9 deletions(-) > > > > diff --git a/lib/dpctl.c b/lib/dpctl.c > > index c501a0cd7..a7a4d8ee8 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; > > @@ -1747,13 +1753,6 @@ dpctl_flush_conntrack(int argc, const char > *argv[], > > args--; > > } > > > > - /* Report error if there is more than one unparsed argument. */ > > - if (args > 1) { > > - ds_put_cstr(&ds, "invalid arguments"); > > - error = EINVAL; > > - goto error; > > - } > > - > > error = opt_dpif_open(argc, argv, dpctl_p, 5, &dpif); > > if (error) { > > dpctl_error(dpctl_p, error, "Cannot open dpif"); > > diff --git a/tests/system-traffic.at b/tests/system-traffic.at > > index 380372430..c6ec61dba 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]) > > > > It looks like we don't have tests for a case where FLUSH_CMD is called > without any arguments while all of them are optional. Could you, please, > add one? > > Best regards, Ilya Maximets. > > Both added in v3. Thanks, Ales
diff --git a/lib/dpctl.c b/lib/dpctl.c index c501a0cd7..a7a4d8ee8 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; @@ -1747,13 +1753,6 @@ dpctl_flush_conntrack(int argc, const char *argv[], args--; } - /* Report error if there is more than one unparsed argument. */ - if (args > 1) { - ds_put_cstr(&ds, "invalid arguments"); - error = EINVAL; - goto error; - } - error = opt_dpif_open(argc, argv, dpctl_p, 5, &dpif); if (error) { dpctl_error(dpctl_p, error, "Cannot open dpif"); diff --git a/tests/system-traffic.at b/tests/system-traffic.at index 380372430..c6ec61dba 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])
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. Signed-off-by: Ales Musil <amusil@redhat.com> --- v2: Add test case. --- lib/dpctl.c | 17 ++++++++--------- tests/system-traffic.at | 2 ++ 2 files changed, 10 insertions(+), 9 deletions(-)