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: Tue, 24 Jul 2018 18:19:35 +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> <2601191342CEEE43887BDE71AB977258DF51CD64@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 EUR03-VE1-obe.outbound.protection.outlook.com (mail-eopbgr50056.outbound.protection.outlook.com [40.107.5.56]) by dpdk.org (Postfix) with ESMTP id 9A916F72 for ; Tue, 24 Jul 2018 14:49:50 +0200 (CEST) In-Reply-To: <2601191342CEEE43887BDE71AB977258DF51CD64@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 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 ; 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, > 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 >>>