On Fri, Mar 10, 2023 at 3:28 PM Ilya Maximets <[email protected]> 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 <[email protected] > > > >> 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 <[email protected]> > >>>> --- > >>>> 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 -- 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
