Re: [dpdk-dev] [PATCH 2/2] examples/ipsec-secgw: fix portmask option parsing

[Akhil Goyal <akhil.goyal@nxp.com>]

Hi Konstantin,

On 7/24/2018 6:07 PM, Ananyev, Konstantin wrote:

> Hi Akhil,
>
>> Hi Konstantin,
>>
>> On 6/22/2018 5:21 PM, Ananyev, Konstantin wrote:
>>
>>>> -----Original Message-----
>>>> From: Akhil Goyal [mailto:akhil.goyal@nxp.com]
>>>> Sent: Friday, June 22, 2018 11:41 AM
>>>> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org
>>>> Cc: Nicolau, Radu <radu.nicolau@intel.com>
>>>> Subject: Re: [dpdk-dev] [PATCH 2/2] examples/ipsec-secgw: fix portmask option parsing
>>>>
>>>>
>>>>
>>>> On 6/22/2018 3:40 PM, Ananyev, Konstantin wrote:
>>>>>> -----Original Message-----
>>>>>> From: Akhil Goyal [mailto:akhil.goyal@nxp.com]
>>>>>> Sent: Friday, June 22, 2018 11:01 AM
>>>>>> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org
>>>>>> Cc: Nicolau, Radu <radu.nicolau@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@nxp.com]
>>>>>>>> Sent: Thursday, June 21, 2018 2:49 PM
>>>>>>>> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org
>>>>>>>> Cc: Nicolau, Radu <radu.nicolau@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@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.
>>> It is still not clear to me why you'd like to prohibit cryptodev_mask==0?
>>> Would anything will be broken?
>>> Konstantin
>> Sorry for delayed response. I missed this one somehow.
>>
>> Nothing is broken,
> Ok
>
>> but it looks very redundant in case of inline modes,
> Why is that?
> Let say I have a crypto device enabled for DPDK, but don't want to use it
> for that particular run.

crypto device will not be used in case of inline, whether you specify the cryptodev_mask or not.

>> and it is not a valid value in case of other modes.
> How that differs from any other invalid crypto-dev mask?
> Let say right now, user can have only one crypto device, but nothing stops him to specify
> --cryptodev_mask=0x10, or so.

That can be an enhancement to the application to validate the cryptodev_mask before using.

But in case it is 0, then it cannot be correct in any of the case, because atleast one crypto device needs to be enabled.

-Akhil

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