| Message ID | 20230309140041.790425-1-amusil@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev,v3,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 Thu, Mar 9, 2023 at 3:00 PM Ales Musil <amusil@redhat.com> 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. > > 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. > --- > lib/dpctl.c | 17 ++++++++--------- > tests/system-traffic.at | 16 ++++++++++++++++ > 2 files changed, 24 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..d5723685a 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]) > > @@ -2503,6 +2505,20 @@ > 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, partial flush by src address in reply > direction > +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]) > ]) > > -- > 2.39.2 > > Ah I forgot to change the comment so this should be applied on top: diff --git a/tests/system-traffic.at b/tests/system-traffic.at index d5723685a..7115a8ef4 100644 --- a/tests/system-traffic.at +++ b/tests/system-traffic.at @@ -2507,7 +2507,7 @@ 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, partial flush by src address in reply direction +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)"]) Thanks, Ales
On Thu, Mar 09, 2023 at 03:00:40PM +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. > > 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. > --- > lib/dpctl.c | 17 ++++++++--------- > tests/system-traffic.at | 16 ++++++++++++++++ > 2 files changed, 24 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; > - } > - Hi Ales, I'm a little unclear on the motivation for the above hunk - what if any rule regarding unparsed arguments should be enforced here? > error = opt_dpif_open(argc, argv, dpctl_p, 5, &dpif); > if (error) { > dpctl_error(dpctl_p, error, "Cannot open dpif"); ...
On Fri, Mar 10, 2023 at 9:58 AM Simon Horman <simon.horman@corigine.com> wrote: > On Thu, Mar 09, 2023 at 03:00:40PM +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. > > > > 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. > > --- > > lib/dpctl.c | 17 ++++++++--------- > > tests/system-traffic.at | 16 ++++++++++++++++ > > 2 files changed, 24 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; > > - } > > - > > Hi Ales, > > I'm a little unclear on the motivation for the above hunk - > what if any rule regarding unparsed arguments should be enforced here? > Hi Simon, it actually shouldn't happen that there is something left unprased, because unknown arguments will be reported by "ofp_ct_tuple_parse". Does that make sense? Thanks, Ales > > > error = opt_dpif_open(argc, argv, dpctl_p, 5, &dpif); > > if (error) { > > dpctl_error(dpctl_p, error, "Cannot open dpif"); > > ... > >
On Fri, Mar 10, 2023 at 10:36:38AM +0100, Ales Musil wrote: > On Fri, Mar 10, 2023 at 9:58 AM Simon Horman <simon.horman@corigine.com> > wrote: > > > On Thu, Mar 09, 2023 at 03:00:40PM +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. > > > > > > 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. > > > --- > > > lib/dpctl.c | 17 ++++++++--------- > > > tests/system-traffic.at | 16 ++++++++++++++++ > > > 2 files changed, 24 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; > > > - } > > > - > > > > Hi Ales, > > > > I'm a little unclear on the motivation for the above hunk - > > what if any rule regarding unparsed arguments should be enforced here? > > > > Hi Simon, > > it actually shouldn't happen that there is something left unprased, > because unknown arguments will be reported by "ofp_ct_tuple_parse". > Does that make sense? Yes, thanks. Perhaps this could be a separate patch? Or this information added to the patch description? Or perhaps it is obvious to everyone except me :)
On 3/10/23 11:14, Simon Horman wrote: > On Fri, Mar 10, 2023 at 10:36:38AM +0100, Ales Musil wrote: >> On Fri, Mar 10, 2023 at 9:58 AM Simon Horman <simon.horman@corigine.com> >> wrote: >> >>> On Thu, Mar 09, 2023 at 03:00:40PM +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. >>>> >>>> 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. >>>> --- >>>> lib/dpctl.c | 17 ++++++++--------- >>>> tests/system-traffic.at | 16 ++++++++++++++++ >>>> 2 files changed, 24 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; >>>> - } >>>> - >>> >>> Hi Ales, >>> >>> I'm a little unclear on the motivation for the above hunk - >>> what if any rule regarding unparsed arguments should be enforced here? >>> >> >> Hi Simon, >> >> it actually shouldn't happen that there is something left unprased, >> because unknown arguments will be reported by "ofp_ct_tuple_parse". >> Does that make sense? > > Yes, thanks. > > Perhaps this could be a separate patch? > Or this information added to the patch description? > Or perhaps it is obvious to everyone except me :) In fact, Simon, you're right and it's not really working as expected. :) I tried following cases: # ovs-appctl dpctl/flush-conntrack system@ovs-system zone=5 'ct_nw_proto=17,ct_tp_src=1' 'ct_nw_proto=17,ct_tp_src=1' qwe "dpctl/flush-conntrack" command takes at most 4 arguments ovs-appctl: ovs-vswitchd: server returned an error <-- OK. # ovs-appctl dpctl/flush-conntrack zone=5 'ct_nw_proto=17,ct_tp_src=1' 'ct_nw_proto=17,ct_tp_src=1' qweovs-vswitchd: datapath not found (Invalid argument) ovs-vswitchd: Cannot open dpif (Invalid argument) ovs-appctl: ovs-vswitchd: server returned an error <-- OK. Failed because we tried to open 'qwe' as a datapath? # ovs-appctl dpctl/flush-conntrack zone=5 'ct_nw_proto=17,ct_tp_src=1' qweovs-vswitchd: field qwe missing value (Invalid argument) ovs-appctl: ovs-vswitchd: server returned an error <-- OK. Failed because we tried to parse 'qwe' as a tuple. # ovs-appctl dpctl/flush-conntrack system@ovs-system 'ct_nw_proto=17,ct_tp_src=1' 'ct_nw_proto=17,ct_tp_src=1' qwe <-- WRONG. This one passed while it shouldn't. So, we should, probably, keep the check after all. The ofp_ct_tuple_parse() is only called twice. Ales, could you also, please, add some negative tests? Best regards, Ilya Maximets.
On 09/03/2023 16:07, Ales Musil wrote: > On Thu, Mar 9, 2023 at 3:00 PM Ales Musil <amusil@redhat.com> 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. >> >> 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. >> --- >> lib/dpctl.c | 17 ++++++++--------- >> tests/system-traffic.at | 16 ++++++++++++++++ >> 2 files changed, 24 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..d5723685a 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]) >> >> @@ -2503,6 +2505,20 @@ >> 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, partial flush by src address in reply >> direction >> +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]) >> ]) >> >> -- >> 2.39.2 >> >> > > Ah I forgot to change the comment so this should be applied on top: > > diff --git a/tests/system-traffic.at b/tests/system-traffic.at > index d5723685a..7115a8ef4 100644 > --- a/tests/system-traffic.at > +++ b/tests/system-traffic.at > @@ -2507,7 +2507,7 @@ 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, partial flush by src address in reply > direction > +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)"]) > > > Thanks, > Ales > thanks for the work. looks good. tested manually the flush. Reviewed-by: Roi Dayan <roid@nvidia.com>
On Fri, Mar 10, 2023 at 3:28 PM Ilya Maximets <i.maximets@ovn.org> wrote: > On 3/10/23 11:14, Simon Horman wrote: > > On Fri, Mar 10, 2023 at 10:36:38AM +0100, Ales Musil wrote: > >> On Fri, Mar 10, 2023 at 9:58 AM Simon Horman <simon.horman@corigine.com > > > >> wrote: > >> > >>> On Thu, Mar 09, 2023 at 03:00:40PM +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. > >>>> > >>>> 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. > >>>> --- > >>>> lib/dpctl.c | 17 ++++++++--------- > >>>> tests/system-traffic.at | 16 ++++++++++++++++ > >>>> 2 files changed, 24 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; > >>>> - } > >>>> - > >>> > >>> Hi Ales, > >>> > >>> I'm a little unclear on the motivation for the above hunk - > >>> what if any rule regarding unparsed arguments should be enforced here? > >>> > >> > >> Hi Simon, > >> > >> it actually shouldn't happen that there is something left unprased, > >> because unknown arguments will be reported by "ofp_ct_tuple_parse". > >> Does that make sense? > > > > Yes, thanks. > > > > Perhaps this could be a separate patch? > > Or this information added to the patch description? > > Or perhaps it is obvious to everyone except me :) > > In fact, Simon, you're right and it's not really working as expected. :) > > I tried following cases: > > # ovs-appctl dpctl/flush-conntrack system@ovs-system zone=5 > 'ct_nw_proto=17,ct_tp_src=1' 'ct_nw_proto=17,ct_tp_src=1' qwe > "dpctl/flush-conntrack" command takes at most 4 arguments > ovs-appctl: ovs-vswitchd: server returned an error > > <-- OK. > > # ovs-appctl dpctl/flush-conntrack zone=5 'ct_nw_proto=17,ct_tp_src=1' > 'ct_nw_proto=17,ct_tp_src=1' qweovs-vswitchd: datapath not found (Invalid > argument) > ovs-vswitchd: Cannot open dpif (Invalid argument) > ovs-appctl: ovs-vswitchd: server returned an error > > <-- OK. Failed because we tried to open 'qwe' as a datapath? > > # ovs-appctl dpctl/flush-conntrack zone=5 'ct_nw_proto=17,ct_tp_src=1' > qweovs-vswitchd: field qwe missing value (Invalid argument) > ovs-appctl: ovs-vswitchd: server returned an error > > <-- OK. Failed because we tried to parse 'qwe' as a tuple. > > # ovs-appctl dpctl/flush-conntrack system@ovs-system > 'ct_nw_proto=17,ct_tp_src=1' 'ct_nw_proto=17,ct_tp_src=1' qwe > > <-- WRONG. This one passed while it shouldn't. > > > So, we should, probably, keep the check after all. The > ofp_ct_tuple_parse() > is only called twice. > > > Ales, could you also, please, add some negative tests? > > Best regards, Ilya Maximets. > > I have added a couple of negative test cases, now it should be covered and working as expected. 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..d5723685a 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]) @@ -2503,6 +2505,20 @@ 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, partial flush by src address in reply direction +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]) ])
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. 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. --- lib/dpctl.c | 17 ++++++++--------- tests/system-traffic.at | 16 ++++++++++++++++ 2 files changed, 24 insertions(+), 9 deletions(-)