On Fri, Mar 3, 2023 at 4:00 PM Roi Dayan <[email protected]> wrote:
>
>
> On 01/03/2023 14:01, Ales Musil wrote:
> > 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
> >
> >
>
> this looks good. maybe the check for args && dp_exists() can be replaced
> with call to dp_arg_exists(argc, argv) as opt_dpif_open() uses.
> It's same just maybe looks nicer.
>
Yeah that's better.
>
> In addition to this I think the check below if there is more than
> one unparsed arg is never reached as the loop finished is args is 0.
> Any invalid args are returned earlier as invalid conntrack tuple field.
> So maybe this check can be removed now?
>
Agreed, I did send a patch for that and added you to cc.
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
