[ovs-dev] [v5 01/11] dpif-netdev: Add command line and function pointer for miniflow extract

[Van Haaren, Harry]

> -----Original Message-----
> From: Eelco Chaudron <echaudro at redhat.com>
> Sent: Tuesday, July 6, 2021 3:19 PM
> To: Ferriter, Cian <cian.ferriter at intel.com>
> Cc: ovs-dev at openvswitch.org; fbl at sysclose.org; i.maximets at ovn.org; Van Haaren,
> Harry <harry.van.haaren at intel.com>; Amber, Kumar <kumar.amber at intel.com>;
> Stokes, Ian <ian.stokes at 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?

<snip remainder of feedback>.

Regards, -Harry