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