> -----Original Message-----
> From: Eelco Chaudron <[email protected]>
> Sent: Tuesday, July 6, 2021 3:19 PM
> To: Ferriter, Cian <[email protected]>
> Cc: [email protected]; [email protected]; [email protected]; Van
> Haaren,
> Harry <[email protected]>; Amber, Kumar <[email protected]>;
> Stokes, Ian <[email protected]>
> 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
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
