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