[dpdk-dev] [PATCH 2/2] examples/ipsec-secgw: fix portmask option parsing
Akhil Goyal
akhil.goyal at nxp.com
Fri Jun 22 12:40:31 CEST 2018
On 6/22/2018 3:40 PM, Ananyev, Konstantin wrote:
>
>> -----Original Message-----
>> From: Akhil Goyal [mailto:akhil.goyal at nxp.com]
>> Sent: Friday, June 22, 2018 11:01 AM
>> To: Ananyev, Konstantin <konstantin.ananyev at intel.com>; dev at dpdk.org
>> Cc: Nicolau, Radu <radu.nicolau at intel.com>
>> Subject: Re: [dpdk-dev] [PATCH 2/2] examples/ipsec-secgw: fix portmask option parsing
>>
>> Hi Konstantin,
>>
>> On 6/21/2018 8:32 PM, Ananyev, Konstantin wrote:
>>
>>> Hi Akhil,
>>>
>>>> -----Original Message-----
>>>> From: Akhil Goyal [mailto:akhil.goyal at nxp.com]
>>>> Sent: Thursday, June 21, 2018 2:49 PM
>>>> To: Ananyev, Konstantin <konstantin.ananyev at intel.com>; dev at dpdk.org
>>>> Cc: Nicolau, Radu <radu.nicolau at intel.com>
>>>> Subject: Re: [dpdk-dev] [PATCH 2/2] examples/ipsec-secgw: fix portmask option parsing
>>>>
>>>> Hi Konstantin,
>>>>
>>>> On 6/5/2018 7:46 PM, Konstantin Ananyev wrote:
>>>>> parse_portmask() returns both portmask value and possible error code
>>>>> as 32-bit integer. That causes some confusion for callers.
>>>>> Split error code and portmask value into two distinct variables.
>>>>> Also allows to run the app with unprotected_port_mask == 0.
>>>> This would also allow cryptodev_mask == 0 to work well which should not be the case.
>>>>
>>>>> Fixes: d299106e8e31 ("examples/ipsec-secgw: add IPsec sample application")
>>>>>
>>>>> Signed-off-by: Konstantin Ananyev <konstantin.ananyev at intel.com>
>>>>> ---
>>>>> examples/ipsec-secgw/ipsec-secgw.c | 29 +++++++++++++++--------------
>>>>> 1 file changed, 15 insertions(+), 14 deletions(-)
>>>>>
>>>>> diff --git a/examples/ipsec-secgw/ipsec-secgw.c b/examples/ipsec-secgw/ipsec-secgw.c
>>>>> index fafb41161..5d7071657 100644
>>>>> --- a/examples/ipsec-secgw/ipsec-secgw.c
>>>>> +++ b/examples/ipsec-secgw/ipsec-secgw.c
>>>>> @@ -972,20 +972,19 @@ print_usage(const char *prgname)
>>>>> }
>>>>>
>>>>> static int32_t
>>>>> -parse_portmask(const char *portmask)
>>>>> +parse_portmask(const char *portmask, uint32_t *pmv)
>>>>> {
>>>>> - char *end = NULL;
>>>>> + char *end;
>>>>> unsigned long pm;
>>>>>
>>>>> /* parse hexadecimal string */
>>>>> + errno = 0;
>>>>> pm = strtoul(portmask, &end, 16);
>>>>> - if ((portmask[0] == '\0') || (end == NULL) || (*end != '\0'))
>>>>> + if (errno != 0 || *end != '\0' || pm > UINT32_MAX)
>>>>> return -1;
>>>>>
>>>>> - if ((pm == 0) && errno)
>>>>> - return -1;
>>>>> -
>>>>> - return pm;
>>>>> + *pmv = pm;
>>>>> + return 0;
>>>>> }
>>>>>
>>>>> static int32_t
>>>>> @@ -1063,6 +1062,7 @@ parse_args(int32_t argc, char **argv)
>>>>> int32_t opt, ret;
>>>>> char **argvopt;
>>>>> int32_t option_index;
>>>>> + uint32_t v;
>>>>> char *prgname = argv[0];
>>>>> int32_t f_present = 0;
>>>>>
>>>>> @@ -1073,8 +1073,8 @@ parse_args(int32_t argc, char **argv)
>>>>>
>>>>> switch (opt) {
>>>>> case 'p':
>>>>> - enabled_port_mask = parse_portmask(optarg);
>>>>> - if (enabled_port_mask == 0) {
>>>>> + ret = parse_portmask(optarg, &enabled_port_mask);
>>>>> + if (ret < 0 || enabled_port_mask == 0) {
>>>>> printf("invalid portmask\n");
>>>>> print_usage(prgname);
>>>>> return -1;
>>>>> @@ -1085,8 +1085,8 @@ parse_args(int32_t argc, char **argv)
>>>>> promiscuous_on = 1;
>>>>> break;
>>>>> case 'u':
>>>>> - unprotected_port_mask = parse_portmask(optarg);
>>>>> - if (unprotected_port_mask == 0) {
>>>>> + ret = parse_portmask(optarg, &unprotected_port_mask);
>>>>> + if (ret < 0) {
>>>>> printf("invalid unprotected portmask\n");
>>>>> print_usage(prgname);
>>>>> return -1;
>>>>> @@ -1147,15 +1147,16 @@ parse_args(int32_t argc, char **argv)
>>>>> single_sa_idx);
>>>>> break;
>>>>> case CMD_LINE_OPT_CRYPTODEV_MASK_NUM:
>>>>> - ret = parse_portmask(optarg);
>>>>> + ret = parse_portmask(optarg, &v);
>>>> I think there is no need for v, enabled_cryptodev_mask can be used instead.
>>> Right now - it can't as enabled_cryptodevmask is uint64_t.
>>> To do what you suggesting we have either downgrade enabled_cryptodevmask 32-bits,
>>> or upgrade enabled_port_mask to 64-bit and change parse_portmask() to accept 64-bit parameter.
>> I am ok with any of the case.
>>
>>>>> if (ret == -1) {
>>>> enabled_cryptodev_mask should not be 0 and should be checked here.
>>> Could you explain a bit more why enabled_cryptodevmask==0 is not allowed?
>> By default, the value of enabled_cryptodevmask is UINT64_MAX, which means all crypto
>> devices are enabled, and if it is marked as 0, then all get disabled which is not
>> correct as we need atleast 1 crypto device in ipsec application.
> Might be user would like to run app with inline ipsec only,
> or have app to work in bypass mode only (no encrypt/decrypt) at all.
> Why that should be considered as a problem?
> Konstantin
Agreed with your point. But in case of inline ipsec, user may not be initializing the crypto device either.
So the cryptodev_mask option would be redundant in that case and it may not give that parameter.
-Akhil
>> So if the user doesn't
>> want to give the cryptodev_mask then he may skip that parameter, but if it is giving,
>> then it cannot be 0.
>>
>>> Konstantin
>>>
>>>
>> -Akhil
More information about the dev
mailing list