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

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

> -----Original Message-----
> From: Akhil Goyal [mailto:akhil.goyal@nxp.com]
> Sent: Tuesday, July 24, 2018 1:50 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 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.

Not sure why is that?
We can have one SA using inline-crypto, while second one using crypto device.

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

Again, not sure why?
As you said above user may choose to use inline-crypto devs only for that particular run.
Konstantin

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