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

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

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.

>   			if (ret == -1) {

enabled_cryptodev_mask should not be 0 and should be checked here.

-Akhil

> -				printf("Invalid argument[portmask]\n");
> +				printf("Invalid argument[%s]\n",
> +					CMD_LINE_OPT_CRYPTODEV_MASK);
>   				print_usage(prgname);
>   				return -1;
>   			}
>   
>   			/* else */
> -			enabled_cryptodev_mask = ret;
> +			enabled_cryptodev_mask = v;
>   			break;
>   		default:
>   			print_usage(prgname);