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

[Eelco Chaudron]

On 7 Jul 2021, at 9:21, Amber, Kumar wrote:

> 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 ?

Looking at the other existing commands, options come before the bridge name, or other switches, so I think we should keep it consistent. The dpif-netdev/pmd-stats-show has a clean implementation, and I think you can use something similar.

>
> I still think the

Guess you sent the email too fast?

>>> <snip remainder of feedback>.
>>>
>>> Regards, -Harry