On Wed, Mar 1, 2023 at 12:34 PM Roi Dayan <[email protected]> wrote:
>
>
> On 01/03/2023 12:53, Roi Dayan via dev wrote:
> >
> >
> > On 16/01/2023 13:45, Ales Musil wrote:
> >> Currently, the CT can be flushed by dpctl only by specifying
> >> the whole 5-tuple. This is not very convenient when there are
> >> only some fields known to the user of CT flush. Add new struct
> >> ofputil_ct_match which represents the generic filtering that can
> >> be done for CT flush. The match is done only on fields that are
> >> non-zero with exception to the icmp fields.
> >>
> >> This allows the filtering just within dpctl, however
> >> it is a preparation for OpenFlow extension.
> >>
> >> Reported-at:
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.redhat.com%2F2120546&data=05%7C01%7Croid%40nvidia.com%7Cbc4c95a2472a4dfbb0a408db1a434035%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C638132648384230198%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=wBA5yCdijnN%2FkcwxncDe1xxHzsqzGQgCRqNRHIMQUHQ%3D&reserved=0
> >> Signed-off-by: Ales Musil <[email protected]>
> >> ---
>
> ...
>
> >> @@ -1707,37 +1708,55 @@ dpctl_flush_conntrack(int argc, const char
> *argv[],
> >> struct dpctl_params *dpctl_p)
> >> {
> >> struct dpif *dpif = NULL;
> >> - struct ct_dpif_tuple tuple, *ptuple = NULL;
> >> + struct ofp_ct_match match = {0};
> >> struct ds ds = DS_EMPTY_INITIALIZER;
> >> uint16_t zone, *pzone = NULL;
> >> int error;
> >> int args = argc - 1;
> >>
> >> - /* Parse ct tuple */
> >> - if (args && ct_dpif_parse_tuple(&tuple, argv[args], &ds)) {
> >> - ptuple = &tuple;
> >> + /* Parse zone. */
> >> + if (args && !strncmp(argv[1], "zone=", 5)) {
> >> + if (!ovs_scan(argv[1], "zone=%"SCNu16, &zone)) {
> >> + ds_put_cstr(&ds, "failed to parse zone");
> >> + error = EINVAL;
> >> + goto error;
> >> + }
> >> + pzone = &zone;
> >> args--;
> >> }
> >>
> >> - /* Parse zone */
> >> - if (args && ovs_scan(argv[args], "zone=%"SCNu16, &zone)) {
> >> - pzone = &zone;
> >> + /* Parse ct tuples. */
> >> + for (int i = 0; i < 2; i++) {
> >> + if (!args) {
> >> + break;
> >> + }
> >> +
> >> + struct ofp_ct_tuple *tuple =
> >> + i ? &match.tuple_reply : &match.tuple_orig;
> >> + const char *arg = argv[argc - args];
> >> +
> >> + if (arg[0] && !ofp_ct_tuple_parse(tuple, arg, &ds,
> &match.ip_proto,
> >> + &match.l3_type)) {
> >> + error = EINVAL;
> >> + goto error;
> >> + }
> >> args--;
> >> }
> >
> > Hi,
> >
> > There is a problem with the change here that you cannot specify now dp.
> >
> > # ovs-appctl dpctl/flush-conntrack system@ovs-system
> > ovs-dpctl: field system@ovs-system missing value (Invalid argument)
> >
> > As I understand it the old logic was parsing from the end and if we left
> > with 1 arg it's considered the dp.
> >
> > The new logic starts from first argument and fails on first failure
> > so we actually never reach the following check if args > 1.
> >
> > A quick fix I tried is to goto error only if args > 1.
> > but this leaves an opening that specifying wrong key fails on the dp arg.
> >
> > # ovs-dpctl flush-conntrack system@ovs-system key=val
> > ovs-dpctl: field system@ovs-system missing value (Invalid argument)
> >
> > I ended up with parsing backwards and checking if the error is on last
> > arg or not.
> >
> > I'll send a fixup for review.
> >
> > Thanks,
> > Roi
> >
>
> Well it worked for manual run I tested but I fail the ct flush testsuite.
>
> 53: conntrack - ct flush FAILED (
> system-traffic.at:2364)
>
> I'm having some trouble following the log maybe you can help and do the
> correct fix.
> i'll also try to look but reporting this for now.
>
> The change I tested is the following:
>
> --- a/lib/dpctl.c
> +++ b/lib/dpctl.c
> @@ -1737,12 +1737,15 @@ dpctl_flush_conntrack(int argc, const char *argv[],
>
> struct ofp_ct_tuple *tuple =
> i ? &match.tuple_reply : &match.tuple_orig;
> - const char *arg = argv[argc - args];
> + const char *arg = argv[args];
>
> if (arg[0] && !ofp_ct_tuple_parse(tuple, arg, &ds,
> &match.ip_proto,
> &match.l3_type)) {
> - error = EINVAL;
> - goto error;
> + if (args > 1) {
> + error = EINVAL;
> + goto error;
> + }
> + break;
> }
> args--;
> }
>
Hi Roi,
thank you for looking into this.
I'm not sure if going from the back would work (it might).
But we can still go from beginning with something like the diff below. (I
haven't tested that).
WDYT?
diff --git a/lib/dpctl.c b/lib/dpctl.c
index c501a0cd7..1174844d3 100644
--- a/lib/dpctl.c
+++ b/lib/dpctl.c
@@ -1717,10 +1717,15 @@ dpctl_flush_conntrack(int argc, const char *argv[],
uint16_t zone, *pzone = NULL;
int error;
int args = argc - 1;
+ int zone_pos = 1;
+ if (args && dp_exists(argv[1])) {
+ 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;
Thanks,
Ales
>
> >>
> >> - /* Report error if there are more than one unparsed argument. */
> >> + /* 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, 4, &dpif);
> >> + error = opt_dpif_open(argc, argv, dpctl_p, 5, &dpif);
> >> if (error) {
> >> + dpctl_error(dpctl_p, error, "Cannot open dpif");
> >> return error;
> >> }
> >>
> ...
>
>
>
--
Ales Musil
Senior Software Engineer - OVN Core
Red Hat EMEA <https://www.redhat.com>
[email protected] IM: amusil
<https://red.ht/sig>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
