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

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

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