> -----Original Message----- > From: Akhil Goyal [mailto:akhil.go...@nxp.com] > Sent: Tuesday, July 24, 2018 1:50 PM > To: Ananyev, Konstantin <konstantin.anan...@intel.com>; dev@dpdk.org > Cc: Nicolau, Radu <radu.nico...@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.go...@nxp.com] > >>>> Sent: Friday, June 22, 2018 11:41 AM > >>>> To: Ananyev, Konstantin <konstantin.anan...@intel.com>; dev@dpdk.org > >>>> Cc: Nicolau, Radu <radu.nico...@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.go...@nxp.com] > >>>>>> Sent: Friday, June 22, 2018 11:01 AM > >>>>>> To: Ananyev, Konstantin <konstantin.anan...@intel.com>; dev@dpdk.org > >>>>>> Cc: Nicolau, Radu <radu.nico...@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.go...@nxp.com] > >>>>>>>> Sent: Thursday, June 21, 2018 2:49 PM > >>>>>>>> To: Ananyev, Konstantin <konstantin.anan...@intel.com>; dev@dpdk.org > >>>>>>>> Cc: Nicolau, Radu <radu.nico...@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.anan...@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 > >>>
