[ovs-dev] [v12 06/11] dpif-netdev: Add packet count and core id paramters for study

Eelco Chaudron echaudro at redhat.com
Thu Jul 15 10:45:53 UTC 2021



On 15 Jul 2021, at 12:31, Eelco Chaudron wrote:

> On 15 Jul 2021, at 12:10, Van Haaren, Harry wrote:
>
>>> -----Original Message-----
>>> From: Eelco Chaudron <echaudro at redhat.com>
>>> Sent: Thursday, July 15, 2021 11:08 AM
>>> To: Van Haaren, Harry <harry.van.haaren at intel.com>
>>> Cc: Amber, Kumar <kumar.amber at intel.com>; ovs-dev at openvswitch.org;
>>> fbl at sysclose.org; i.maximets at ovn.org; Ferriter, Cian 
>>> <cian.ferriter at intel.com>;
>>> Stokes, Ian <ian.stokes at intel.com>
>>> Subject: Re: [v12 06/11] dpif-netdev: Add packet count and core id 
>>> paramters for
>>> study
>>
>> <snip>
>>
>>>> Aha, yes OK I see what you're suggesting clearly now.
>>>>
>>>> Personally I liked the "only process extra args if study" trick (by 
>>>> "indenting" it a
>>> level),
>>>> but I'll rework to your suggestion here for simplicity/consistency.
>>>
>>> Thanks looking forward to v13, a nice number to sign off on ;)
>>
>> Hah yeah. To keep you interested, below the diff to fixup the new 
>> behaviour.
>> Note that study_count must be initialized to the default value, as it 
>> is an optional
>> argument and there is no "} else {" clause to handle the default 
>> value anymore.
>>
>> v13 on its way, -Harry
>>
>>
>> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
>> index 14ccca7a03..476678e9fa 100644
>> --- a/lib/dpif-netdev.c
>> +++ b/lib/dpif-netdev.c
>> @@ -1088,7 +1088,7 @@ dpif_miniflow_extract_impl_set(struct 
>> unixctl_conn *conn, int argc,
>>      const char *mfex_name = NULL;
>>      const char *reply_str = NULL;
>>      struct ds reply = DS_EMPTY_INITIALIZER;
>> -    unsigned int study_count = 0;
>> +    unsigned int study_count = MFEX_MAX_PKT_COUNT;
>>      int err;
>>      struct shash_node *node;
>>
>> @@ -1122,23 +1122,17 @@ dpif_miniflow_extract_impl_set(struct 
>> unixctl_conn *conn, int argc,
>>              argc -= 1;
>>              argv += 1;
>>
>> -            /* If name is study and more args, parse study_count 
>> value. */
>> -            if (strcmp("study", mfex_name) == 0) {
>> -                if (argc >= 2) {
>> -                    if (!str_to_uint(argv[1], 10, &study_count) ||
>> -                            (study_count == 0)) {
>> -                        ds_put_format(&reply,
>> -                            "Error: Invalid study_pkt_cnt value: 
>> %s.\n",
>> -                            argv[1]);
>> -                        goto error;
>> -                    }
>> -
>> -                    argc -= 1;
>> -                    argv += 1;
>> -                } else {
>> -                    study_count = MFEX_MAX_PKT_COUNT;
>> -                }

Now I remember what I was thinking ;)

	You do the compare here:
        if (strcmp("study", mfex_name))
            study_count = MFEX_MAX_PKT_COUNT;


This way you can use !study_count in the rest of the code.

>> +        /* If name is study and more args exist, parse study_count 
>> value. */
>> +        } else if (mfex_name && strcmp("study", mfex_name) == 0) {
>> +            if (!str_to_uint(argv[1], 10, &study_count) ||
>> +                    (study_count == 0)) {
>> +                ds_put_format(&reply,
>> +                    "Error: Invalid study_pkt_cnt value: %s.\n", 
>> argv[1]);
>> +                goto error;
>>              }
>> +
>> +            argc -= 1;
>> +            argv += 1;
>>          } else {
>>              ds_put_format(&reply, "Error: unknown argument %s.\n", 
>> argv[1]);
>>                  goto error;
>
> Note that that might mess up the output at the end on successful 
> completion, and setting the value globally.


More information about the dev mailing list