On 6 Jul 2021, at 17:32, Van Haaren, Harry wrote:
>> -----Original Message----- >> From: Eelco Chaudron <echau...@redhat.com> >> Sent: Tuesday, July 6, 2021 3:19 PM >> To: Ferriter, Cian <cian.ferri...@intel.com> >> Cc: ovs-dev@openvswitch.org; f...@sysclose.org; i.maxim...@ovn.org; Van >> Haaren, >> Harry <harry.van.haa...@intel.com>; Amber, Kumar <kumar.am...@intel.com>; >> Stokes, Ian <ian.sto...@intel.com> >> Subject: Re: [v5 01/11] dpif-netdev: Add command line and function pointer >> for >> miniflow extract >> >> See comments inline... > > <snip many comments, Amber will address smaller/code-level changes> > >>> + return; >>> + } >> >> >> Argument handling is not as it should be, see my previous comment. I think >> the >> packets count should only be available for the study option (this might not >> be the >> correct patch, but just want to make sure it’s addressed, and I do not >> forget). >> >> So as an example this looks odd trying to set it for a specific PMD: >> >> $ ovs-appctl dpif-netdev/miniflow-parser-set autovalidator 15 1 >> Miniflow implementation set to autovalidator, on pmd thread 1 >> >> Why do I have to put in the dummy value 15. Here is a quote from my previous >> comment: >> >> “ >> We also might need to re-think the command to make sure >> packet_count_to_study >> is only needed for the study command. >> So the help text might become something like: >> >> dpif-netdev/miniflow-parser-set {miniflow_implementation_name | study >> [pkt_cnt]} [dp] [pmd_core] > > > I don't particularly like the "insert extra variable" with PMD_core moving up > an index if study is used. > Special casing specific implementations like that (if study then change > indexes) is nasty. > > (Note that based on Flavio's feedback the [dp] argument was removed.) > > Thoughts on the this suggestion: > $ dpif-netdev/miniflow-parser-set miniflow_implementation_name [pmd_core] > [pkt_cnt] > > Notes: > 1) All arguments are positional, optional arguments at the end > 2) Based on "power-user-ness", the command required will get longer > - simple usage is simple, with just $ miniflow-parser-set <impl_name> > 3) The worst-part is that to specify [pkt_cnt] to study, it also requires > setting [pmd_core]. Given [pkt_cnt] is a power-user option, I think this is > the right compromise. > > Agree to implement & merge the above? You are right, and your suggestion eases the general use case but makes the pkt_cnt option hard to use, as you have to execute the command for each PMD. I was looking at other commands with the same problem, and they use a -pmd keyword approach. Some examples: dpif-netdev/pmd-rxq-show [-pmd core] [dp] dpif-netdev/pmd-stats-clear [-pmd core] [dp] dpif-netdev/pmd-stats-show [-pmd core] [dp] Guess we can do the same here: dpif-netdev/miniflow-parser-set [-pmd core] miniflow_implementation_name [pkt_cnt] > <snip remainder of feedback>. > > Regards, -Harry _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev