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. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
