> -----Original Message----- > From: dev [mailto:dev-boun...@dpdk.org] On Behalf Of Akhil Goyal > Sent: Thursday, July 5, 2018 10:03 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/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, but it looks very redundant in case of inline modes, and > it is > not a valid value in case of other modes.
Any further comments? Thanks, Pablo > > > > >> -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 > >
