From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 56DDBA00C3; Wed, 15 Dec 2021 11:11:45 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id CD21440688; Wed, 15 Dec 2021 11:11:44 +0100 (CET) Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by mails.dpdk.org (Postfix) with ESMTP id C7CC840041 for ; Wed, 15 Dec 2021 11:11:42 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1639563103; x=1671099103; h=date:from:to:cc:subject:message-id:references: mime-version:content-transfer-encoding:in-reply-to; bh=uAQMLDDdlRbtbtz2HA0y1PonYJMxpt7tjsR7a/I3t9k=; b=lOTUPLycw+3S2yuDC7Tu8EqdhoL3IpuaDoSFodlp5SBjqOfPgz8V4SqX EKGmsRp63SI+/tPrmhj9lIpD/bcP8KTuFB7Xk7on7v2k9SOjkuSdg+e3l L5mipyismWR1B2xu1Why5xa5ftiFvtgQuf28wt4TF2/22VjIF3+1IKWcy qzFYFIhlb2KFT1fGq8IY2sbjgkcPEdjlnLgCxm4TxluK47SN5LWICgJc6 EFhvd37QkZB5/YEIzdcMrF2kuIJFSITnt6jHBlclWaRt/tyTROsxsmBM5 hEu3lQ5pba1y5Fw/S1XL4RXthimynigVcrr73LKGcnbxC1cyGByVV1NAb Q==; X-IronPort-AV: E=McAfee;i="6200,9189,10198"; a="226475805" X-IronPort-AV: E=Sophos;i="5.88,207,1635231600"; d="scan'208";a="226475805" Received: from orsmga006.jf.intel.com ([10.7.209.51]) by orsmga101.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 15 Dec 2021 02:11:41 -0800 X-IronPort-AV: E=Sophos;i="5.88,207,1635231600"; d="scan'208";a="465528445" Received: from bricha3-mobl.ger.corp.intel.com ([10.252.7.115]) by orsmga006-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-SHA; 15 Dec 2021 02:11:39 -0800 Date: Wed, 15 Dec 2021 10:11:36 +0000 From: Bruce Richardson To: Morten =?iso-8859-1?Q?Br=F8rup?= Cc: Ronan Randles , dev@dpdk.org, harry.van.haaren@intel.com Subject: Re: [PATCH 01/12] net: add string to IPv4 parse function Message-ID: References: <20211214141242.3383831-1-ronan.randles@intel.com> <20211214141242.3383831-2-ronan.randles@intel.com> <98CBD80474FA8B44BF855DF32C47DC35D86D6E@smartserver.smartshare.dk> <98CBD80474FA8B44BF855DF32C47DC35D86D77@smartserver.smartshare.dk> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <98CBD80474FA8B44BF855DF32C47DC35D86D77@smartserver.smartshare.dk> X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org On Wed, Dec 15, 2021 at 10:35:44AM +0100, Morten Brørup wrote: > > From: Bruce Richardson [mailto:bruce.richardson@intel.com] > > Sent: Wednesday, 15 December 2021 10.27 > > > > On Tue, Dec 14, 2021 at 06:31:06PM +0100, Morten Brørup wrote: > > > > From: Ronan Randles [mailto:ronan.randles@intel.com] > > > > Sent: Tuesday, 14 December 2021 15.13 > > > > > > > > Added function that accepts ip string as a parameter and returns an > > ip > > > > address represented by a uint32_t. Relevant unit test for this > > function > > > > is also included. > > > > > > > > Signed-off-by: Harry van Haaren > > > > Signed-off-by: Ronan Randles > > > > --- > > > > > > [snip] > > > > > > > diff --git a/lib/net/rte_ip.h b/lib/net/rte_ip.h > > > > index c575250852..188054fda4 100644 > > > > --- a/lib/net/rte_ip.h > > > > +++ b/lib/net/rte_ip.h > > > > @@ -426,6 +426,24 @@ rte_ipv4_udptcp_cksum_verify(const struct > > > > rte_ipv4_hdr *ipv4_hdr, > > > > return 0; > > > > } > > > > > > > > +/** > > > > + * IP address parser. > > > > + * > > > > + * @param src_ip > > > > + * The IP address to be parsed. > > > > + * @param output_addr > > > > + * The array in which the parsed digits will be saved. > > > > + * > > > > + * @retval 0 > > > > + * Success. > > > > + * @retval -1 > > > > + * Failure due to invalid input arguments. > > > > + */ > > > > + > > > > +__rte_experimental > > > > +int32_t > > > > +rte_ip_parse_addr(const char *src_ip, uint32_t *output_addr); > > > > + > > > > > > Good initiative! > > > > > > This should set a precedent for to/from string functions, so be > > careful about names and calling conventions. > > > > > > I have a few suggestions: > > > > > > The function should take a parameter to tell if the input string must > > be zero-terminated or not. This is highly useful for parsing subnet > > strings (e.g. "192.0.2.0/24") and IP range strings (e.g. "192.0.2.2- > > 192.0.2.253"). > > > > > > The return value should be the number of characters read from the > > input string, and still -1 on error. With this modification, also > > consider using the return type ssize_t instead of int32_t. > > > > > Interesting point on the "must be zero-terminated" parameter. However, > > if > > the return value is the number of characters read, then the user can > > check > > for null-termination very easily after the call, and not need the > > parameter. Therefore, I would suggest having the function always assume > > chars after the address and let the user check for null if needed > > afterwards, to keep function simpler. > > That would require additional lines of code in the application. I would rather have that additional code inside the utility function. There is no need to keep the function simple. > Well, it's only an extra item in the error-check conditional, but point taken. I have no strong opinion here.