From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id C25A4A04AF; Tue, 11 Aug 2020 14:53:19 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 03D261C022; Tue, 11 Aug 2020 14:53:19 +0200 (CEST) Received: from relay4-d.mail.gandi.net (relay4-d.mail.gandi.net [217.70.183.196]) by dpdk.org (Postfix) with ESMTP id 59EBE1C01F for ; Tue, 11 Aug 2020 14:53:18 +0200 (CEST) X-Originating-IP: 90.92.205.40 Received: from u256.net (lfbn-idf2-1-1144-40.w90-92.abo.wanadoo.fr [90.92.205.40]) (Authenticated sender: grive@u256.net) by relay4-d.mail.gandi.net (Postfix) with ESMTPSA id A6C8BE0005; Tue, 11 Aug 2020 12:53:16 +0000 (UTC) Date: Tue, 11 Aug 2020 14:53:11 +0200 From: =?utf-8?Q?Ga=C3=ABtan?= Rivet To: Ivan Dyukov Cc: dev@dpdk.org, thomas@monjalon.net, ferruh.yigit@intel.com, stephen@networkplumber.org, Morten =?utf-8?Q?Br=C3=B8rup?= Message-ID: <20200811125311.x6aqxraobb42tcib@u256.net> References: <20200427095737.11082-1-i.dyukov@samsung.com> <20200811085246.28735-1-i.dyukov@samsung.com> <20200811085246.28735-3-i.dyukov@samsung.com> <20200811110241.tfucvkcgn7yhlvoo@u256.net> <60cb10d2-a8e9-d4ac-b0d6-ab9c26970990@samsung.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <60cb10d2-a8e9-d4ac-b0d6-ab9c26970990@samsung.com> Subject: Re: [dpdk-dev] [PATCH v9 02/24] ethdev: add a link status text representation X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On 11/08/20 15:48 +0300, Ivan Dyukov wrote: > 11.08.2020 14:02, Gaëtan Rivet пишет: > > On 11/08/20 11:52 +0300, Ivan Dyukov wrote: > >> Link status structure keeps complicated values which are hard to > >> represent to end user. e.g. link_speed has INT_MAX value which > >> means that speed is unknown, link_duplex equal to 0 means > >> 'half-duplex' etc. To simplify processing of the values > >> in application, new dpdk function is introduced. > >> > >> This commit adds function which treat link status structure > >> and format it to text representation. User may create custom > >> link status string using format string. If format string is NULL, > >> the function construct standard link status string. > >> > >> Signed-off-by: Ivan Dyukov > > Hello Ivan, > > > > I don't see much difference for this patch, from what I read in previous > > thread on the principle it does not seem motivated enough? > > > > I'd have a few nits on the implementation, but on the principle: I think > > this can be an incentive to get properly formatted port status strings. > > > > The API is a little awkward however, and it is definitely complex to > > maintain a format-based function. I think we could do without. > > > > I've tried a smaller alternative. > > > > + simpler to use. > > + simpler to maintain. > > + safer in general. > > + no need to declare local string to store intermediate output. > > > > - one ugly macro. > > It would be good for all values except link_speed because link speed > should be formated using sprintf e.g. > > char str[15]; > > ... > > char *rte_eth_link_speed_str(uint32_t link_speed) { > > if (link_speed == UINT32_MAX) > > return "Unknown"; > > else > > snprintf(str,sizeof(str),"%d",link_speed); > > return str; > > } > > so rte_eth_link_speed_str will require some global string, not local. Sorry I don't understand your point, the implementation below works? > > +/** > > + * Missing: doc. > > + */ > > +static inline const char * > > +rte_eth_link_speed_str(uint32_t speed) > > +{ > > + struct { > > + const char *str; > > + uint32_t speed; > > + } speed_str_map[] = { > > + { "Unknown", ETH_SPEED_NUM_NONE }, > > + { "10 Mbps", ETH_SPEED_NUM_10M }, > > + { "100 Mbps", ETH_SPEED_NUM_100M }, > > + { "1 Gbps", ETH_SPEED_NUM_1G }, > > + { "2.5 Gbps", ETH_SPEED_NUM_2_5G }, > > + { "5 Gbps", ETH_SPEED_NUM_5G }, > > + { "10 Gbps", ETH_SPEED_NUM_10G }, > > + { "20 Gbps", ETH_SPEED_NUM_20G }, > > + { "25 Gbps", ETH_SPEED_NUM_25G }, > > + { "40 Gbps", ETH_SPEED_NUM_40G }, > > + { "50 Gbps", ETH_SPEED_NUM_50G }, > > + { "56 Gbps", ETH_SPEED_NUM_56G }, > > + { "100 Gbps", ETH_SPEED_NUM_100G }, > > + { "200 Gbps", ETH_SPEED_NUM_200G }, > > + }; > > + size_t i; > > + > > + for (i = 0; i < RTE_DIM(speed_str_map); i++) { > > + if (speed == speed_str_map[i].speed) > > + return speed_str_map[i].str; > > + } > > + > > + return speed_str_map[0].str; > > +} -- Gaëtan