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

["Iremonger, Bernard" <bernard.iremonger@intel.com>]

Hi Konstantin,

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Konstantin Ananyev
> Sent: Tuesday, June 5, 2018 3:16 PM
> To: dev@dpdk.org; dev@dpdk.org
> Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Nicolau, Radu
> <radu.nicolau@intel.com>
> Subject: [dpdk-dev] [PATCH 2/2] examples/ipsec-secgw: fix portmask option
> parsing
> 
> 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.
> 
> 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;

The variable "v" seems a bit cryptic to me, how about "pmv" or "mask_val" or "value"?
 
>  	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);
>  			if (ret == -1) {
> -				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);
> --
> 2.13.6

Regards,

Bernard.