From mboxrd@z Thu Jan 1 00:00:00 1970 From: Akhil Goyal Subject: Re: [PATCH 2/2] examples/ipsec-secgw: fix portmask option parsing Date: Thu, 5 Jul 2018 14:33:25 +0530 Message-ID: References: <1528208163-31560-1-git-send-email-konstantin.ananyev@intel.com> <1528208163-31560-2-git-send-email-konstantin.ananyev@intel.com> <36d2a552-e043-7451-ed66-70d13f0f6fe6@nxp.com> <2601191342CEEE43887BDE71AB977258C0C4009C@irsmsx105.ger.corp.intel.com> <2ff9fe7a-3965-ca79-e5e3-9890cc6ce49d@nxp.com> <2601191342CEEE43887BDE71AB977258C0C40347@irsmsx105.ger.corp.intel.com> <2601191342CEEE43887BDE71AB977258C0C406CF@irsmsx105.ger.corp.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Cc: "Nicolau, Radu" To: "Ananyev, Konstantin" , "dev@dpdk.org" Return-path: Received: from EUR02-VE1-obe.outbound.protection.outlook.com (mail-eopbgr20040.outbound.protection.outlook.com [40.107.2.40]) by dpdk.org (Postfix) with ESMTP id 13FE61C399 for ; Thu, 5 Jul 2018 11:03:42 +0200 (CEST) In-Reply-To: <2601191342CEEE43887BDE71AB977258C0C406CF@irsmsx105.ger.corp.intel.com> Content-Language: en-US List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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 ; dev@dpdk.org >> Cc: Nicolau, Radu >> 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 ; dev@dpdk.org >>>> Cc: Nicolau, Radu >>>> 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 ; dev@dpdk.org >>>>>> Cc: Nicolau, Radu >>>>>> 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 >>>>>>> --- >>>>>>> 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 >