Hi Eelco
Pls find my comments inline.
> >> 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]
>
We can certainly do that but there is a problem making [-pmd-core] first
parameter makes it required before every set command which should not be the
case as this is
Effectively still optional and it doesn’t look pleasant and will introduce
if-else kind of dirty code in a already complex set command ?
I still think the
> > <snip remainder of feedback>.
> >
> > Regards, -Harry
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
