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

["Ananyev, Konstantin" <konstantin.ananyev@intel.com>]

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. 

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

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