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