[dpdk-dev,2/2] examples/ipsec-secgw: fix portmask option parsing
Checks
Commit Message
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.
Fixes: d299106e8e31 ("examples/ipsec-secgw: add IPsec sample application")
Signed-off-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
---
examples/ipsec-secgw/ipsec-secgw.c | 29 +++++++++++++++--------------
1 file changed, 15 insertions(+), 14 deletions(-)
Comments
Hi Konstantin,
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Konstantin Ananyev
> Sent: Tuesday, June 5, 2018 3:16 PM
> To: dev@dpdk.org; dev@dpdk.org
> Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Nicolau, Radu
> <radu.nicolau@intel.com>
> Subject: [dpdk-dev] [PATCH 2/2] examples/ipsec-secgw: fix portmask option
> parsing
>
> 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.
>
> Fixes: d299106e8e31 ("examples/ipsec-secgw: add IPsec sample application")
>
> Signed-off-by: Konstantin Ananyev <konstantin.ananyev@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;
The variable "v" seems a bit cryptic to me, how about "pmv" or "mask_val" or "value"?
> 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);
> if (ret == -1) {
> - printf("Invalid argument[portmask]\n");
> + printf("Invalid argument[%s]\n",
> + CMD_LINE_OPT_CRYPTODEV_MASK);
> print_usage(prgname);
> return -1;
> }
>
> /* else */
> - enabled_cryptodev_mask = ret;
> + enabled_cryptodev_mask = v;
> break;
> default:
> print_usage(prgname);
> --
> 2.13.6
Regards,
Bernard.
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.ananyev@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.
> if (ret == -1) {
enabled_cryptodev_mask should not be 0 and should be checked here.
-Akhil
> - printf("Invalid argument[portmask]\n");
> + printf("Invalid argument[%s]\n",
> + CMD_LINE_OPT_CRYPTODEV_MASK);
> print_usage(prgname);
> return -1;
> }
>
> /* else */
> - enabled_cryptodev_mask = ret;
> + enabled_cryptodev_mask = v;
> break;
> default:
> print_usage(prgname);
Hi Akhil,
> -----Original Message-----
> From: Akhil Goyal [mailto:akhil.goyal@nxp.com]
> Sent: Thursday, June 21, 2018 2:49 PM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org
> Cc: Nicolau, Radu <radu.nicolau@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.ananyev@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.
>
> > 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?
Konstantin
Hi Konstantin,
On 6/21/2018 8:32 PM, Ananyev, Konstantin wrote:
> Hi Akhil,
>
>> -----Original Message-----
>> From: Akhil Goyal [mailto:akhil.goyal@nxp.com]
>> Sent: Thursday, June 21, 2018 2:49 PM
>> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org
>> Cc: Nicolau, Radu <radu.nicolau@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.ananyev@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. 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
> -----Original Message-----
> From: Akhil Goyal [mailto:akhil.goyal@nxp.com]
> Sent: Friday, June 22, 2018 11:01 AM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org
> Cc: Nicolau, Radu <radu.nicolau@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.goyal@nxp.com]
> >> Sent: Thursday, June 21, 2018 2:49 PM
> >> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org
> >> Cc: Nicolau, Radu <radu.nicolau@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.ananyev@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
On 6/22/2018 3:40 PM, Ananyev, Konstantin wrote:
>
>> -----Original Message-----
>> From: Akhil Goyal [mailto:akhil.goyal@nxp.com]
>> Sent: Friday, June 22, 2018 11:01 AM
>> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org
>> Cc: Nicolau, Radu <radu.nicolau@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.goyal@nxp.com]
>>>> Sent: Thursday, June 21, 2018 2:49 PM
>>>> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org
>>>> Cc: Nicolau, Radu <radu.nicolau@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.ananyev@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.
-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
> -----Original Message-----
> From: Akhil Goyal [mailto:akhil.goyal@nxp.com]
> Sent: Friday, June 22, 2018 11:41 AM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org
> Cc: Nicolau, Radu <radu.nicolau@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.goyal@nxp.com]
> >> Sent: Friday, June 22, 2018 11:01 AM
> >> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org
> >> Cc: Nicolau, Radu <radu.nicolau@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.goyal@nxp.com]
> >>>> Sent: Thursday, June 21, 2018 2:49 PM
> >>>> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org
> >>>> Cc: Nicolau, Radu <radu.nicolau@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.ananyev@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
Hi Konstantin,
On 6/22/2018 5:21 PM, Ananyev, Konstantin wrote:
>
>> -----Original Message-----
>> From: Akhil Goyal [mailto:akhil.goyal@nxp.com]
>> Sent: Friday, June 22, 2018 11:41 AM
>> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org
>> Cc: Nicolau, Radu <radu.nicolau@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.goyal@nxp.com]
>>>> Sent: Friday, June 22, 2018 11:01 AM
>>>> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org
>>>> Cc: Nicolau, Radu <radu.nicolau@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.goyal@nxp.com]
>>>>>> Sent: Thursday, June 21, 2018 2:49 PM
>>>>>> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org
>>>>>> Cc: Nicolau, Radu <radu.nicolau@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.ananyev@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.
>
>> -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
>
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Akhil Goyal
> Sent: Thursday, July 5, 2018 10:03 AM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org
> Cc: Nicolau, Radu <radu.nicolau@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.goyal@nxp.com]
> >> Sent: Friday, June 22, 2018 11:41 AM
> >> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org
> >> Cc: Nicolau, Radu <radu.nicolau@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.goyal@nxp.com]
> >>>> Sent: Friday, June 22, 2018 11:01 AM
> >>>> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>;
> >>>> dev@dpdk.org
> >>>> Cc: Nicolau, Radu <radu.nicolau@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.goyal@nxp.com]
> >>>>>> Sent: Thursday, June 21, 2018 2:49 PM
> >>>>>> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>;
> >>>>>> dev@dpdk.org
> >>>>>> Cc: Nicolau, Radu <radu.nicolau@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.ananyev@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
> >
Hi Akhil,
>
> Hi Konstantin,
>
> On 6/22/2018 5:21 PM, Ananyev, Konstantin wrote:
>
> >
> >> -----Original Message-----
> >> From: Akhil Goyal [mailto:akhil.goyal@nxp.com]
> >> Sent: Friday, June 22, 2018 11:41 AM
> >> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org
> >> Cc: Nicolau, Radu <radu.nicolau@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.goyal@nxp.com]
> >>>> Sent: Friday, June 22, 2018 11:01 AM
> >>>> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org
> >>>> Cc: Nicolau, Radu <radu.nicolau@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.goyal@nxp.com]
> >>>>>> Sent: Thursday, June 21, 2018 2:49 PM
> >>>>>> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org
> >>>>>> Cc: Nicolau, Radu <radu.nicolau@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.ananyev@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.
>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.
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
> >
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.goyal@nxp.com]
>>>> Sent: Friday, June 22, 2018 11:41 AM
>>>> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org
>>>> Cc: Nicolau, Radu <radu.nicolau@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.goyal@nxp.com]
>>>>>> Sent: Friday, June 22, 2018 11:01 AM
>>>>>> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org
>>>>>> Cc: Nicolau, Radu <radu.nicolau@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.goyal@nxp.com]
>>>>>>>> Sent: Thursday, June 21, 2018 2:49 PM
>>>>>>>> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org
>>>>>>>> Cc: Nicolau, Radu <radu.nicolau@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.ananyev@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
>>>
> -----Original Message-----
> From: Akhil Goyal [mailto:akhil.goyal@nxp.com]
> Sent: Tuesday, July 24, 2018 1:50 PM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org
> Cc: Nicolau, Radu <radu.nicolau@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.goyal@nxp.com]
> >>>> Sent: Friday, June 22, 2018 11:41 AM
> >>>> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org
> >>>> Cc: Nicolau, Radu <radu.nicolau@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.goyal@nxp.com]
> >>>>>> Sent: Friday, June 22, 2018 11:01 AM
> >>>>>> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org
> >>>>>> Cc: Nicolau, Radu <radu.nicolau@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.goyal@nxp.com]
> >>>>>>>> Sent: Thursday, June 21, 2018 2:49 PM
> >>>>>>>> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org
> >>>>>>>> Cc: Nicolau, Radu <radu.nicolau@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.ananyev@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
> >>>
@@ -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);
if (ret == -1) {
- printf("Invalid argument[portmask]\n");
+ printf("Invalid argument[%s]\n",
+ CMD_LINE_OPT_CRYPTODEV_MASK);
print_usage(prgname);
return -1;
}
/* else */
- enabled_cryptodev_mask = ret;
+ enabled_cryptodev_mask = v;
break;
default:
print_usage(prgname);