DPDK patches and discussions
 help / color / mirror / Atom feed
From: Akhil Goyal <akhil.goyal@nxp.com>
To: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>,
	"dev@dpdk.org" <dev@dpdk.org>
Cc: "Nicolau, Radu" <radu.nicolau@intel.com>
Subject: Re: [dpdk-dev] [PATCH 2/2] examples/ipsec-secgw: fix portmask option parsing
Date: Thu, 5 Jul 2018 14:33:25 +0530	[thread overview]
Message-ID: <f82a1e15-559e-d0f0-b6e8-cc327d49e46e@nxp.com> (raw)
In-Reply-To: <2601191342CEEE43887BDE71AB977258C0C406CF@irsmsx105.ger.corp.intel.com>

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
>

  reply	other threads:[~2018-07-05  9:03 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-06-05 14:16 [dpdk-dev] [PATCH 1/2] examples/ipsec-secgw: fix bypass rule processing for outbound port Konstantin Ananyev
2018-06-05 14:16 ` [dpdk-dev] [PATCH 2/2] examples/ipsec-secgw: fix portmask option parsing Konstantin Ananyev
2018-06-05 15:36   ` Iremonger, Bernard
2018-06-21 13:48   ` Akhil Goyal
2018-06-21 15:02     ` Ananyev, Konstantin
2018-06-22 10:00       ` Akhil Goyal
2018-06-22 10:10         ` Ananyev, Konstantin
2018-06-22 10:40           ` Akhil Goyal
2018-06-22 11:51             ` Ananyev, Konstantin
2018-07-05  9:03               ` Akhil Goyal [this message]
2018-07-24  8:48                 ` De Lara Guarch, Pablo
2018-07-24 12:37                 ` Ananyev, Konstantin
2018-07-24 12:49                   ` Akhil Goyal
2018-07-24 13:04                     ` Ananyev, Konstantin
2018-06-21 13:25 ` [dpdk-dev] [PATCH 1/2] examples/ipsec-secgw: fix bypass rule processing for outbound port Akhil Goyal
2018-07-24 16:30   ` De Lara Guarch, Pablo

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=f82a1e15-559e-d0f0-b6e8-cc327d49e46e@nxp.com \
    --to=akhil.goyal@nxp.com \
    --cc=dev@dpdk.org \
    --cc=konstantin.ananyev@intel.com \
    --cc=radu.nicolau@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).