From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <dev-bounces@dpdk.org>
Received: from dpdk.org (dpdk.org [92.243.14.124])
	by inbox.dpdk.org (Postfix) with ESMTP id E4DD1A04F1;
	Thu, 18 Jun 2020 12:08:36 +0200 (CEST)
Received: from [92.243.14.124] (localhost [127.0.0.1])
	by dpdk.org (Postfix) with ESMTP id 7FD0C1BEC4;
	Thu, 18 Jun 2020 12:08:36 +0200 (CEST)
Received: from mailout2.w1.samsung.com (mailout2.w1.samsung.com
 [210.118.77.12]) by dpdk.org (Postfix) with ESMTP id 068813B5
 for <dev@dpdk.org>; Thu, 18 Jun 2020 12:08:34 +0200 (CEST)
Received: from eucas1p1.samsung.com (unknown [182.198.249.206])
 by mailout2.w1.samsung.com (KnoxPortal) with ESMTP id
 20200618100834euoutp0276455cce162b8a948f355c769dedbc62~Zm39Tzp4f2461924619euoutp02M
 for <dev@dpdk.org>; Thu, 18 Jun 2020 10:08:34 +0000 (GMT)
DKIM-Filter: OpenDKIM Filter v2.11.0 mailout2.w1.samsung.com
 20200618100834euoutp0276455cce162b8a948f355c769dedbc62~Zm39Tzp4f2461924619euoutp02M
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=samsung.com;
 s=mail20170921; t=1592474914;
 bh=dSePoWE6t9nVqAiRW//VkjI0gYbGzNuT++wqoeZ/QSQ=;
 h=Subject:To:From:Date:In-Reply-To:References:From;
 b=BS09AKUAfMt0f9muyMQpASOkN4YBeFGhzI5bueiXrZgjtfaS5uHLhbystSWgor6aB
 VMA+J8wxI/6uxfjGGCEH+foyGU4N5bz9RQyEeil8msiVir4y+I4YRclNk6sL8OMsUA
 iLI7/i4ls0/Rjg5P1teOCXwYH2iDpsXnAWP89et8=
Received: from eusmges2new.samsung.com (unknown [203.254.199.244]) by
 eucas1p2.samsung.com (KnoxPortal) with ESMTP id
 20200618100834eucas1p201cce1d0085da0de695ca7c24696a220~Zm39CpWhd0581105811eucas1p2_;
 Thu, 18 Jun 2020 10:08:34 +0000 (GMT)
Received: from eucas1p2.samsung.com ( [182.198.249.207]) by
 eusmges2new.samsung.com (EUCPMTA) with SMTP id 71.88.60679.12D3BEE5; Thu, 18
 Jun 2020 11:08:34 +0100 (BST)
Received: from eusmtrp1.samsung.com (unknown [182.198.249.138]) by
 eucas1p1.samsung.com (KnoxPortal) with ESMTPA id
 20200618100833eucas1p15079944aa0fac47bf533d772d12d93e0~Zm38qvlM42770827708eucas1p13;
 Thu, 18 Jun 2020 10:08:33 +0000 (GMT)
Received: from eusmgms1.samsung.com (unknown [182.198.249.179]) by
 eusmtrp1.samsung.com (KnoxPortal) with ESMTP id
 20200618100833eusmtrp10e405ab600bebcc62bbece363f4795fe~Zm38kZ1X92452324523eusmtrp1_;
 Thu, 18 Jun 2020 10:08:33 +0000 (GMT)
X-AuditID: cbfec7f4-0e5ff7000001ed07-76-5eeb3d21e833
Received: from eusmtip2.samsung.com ( [203.254.199.222]) by
 eusmgms1.samsung.com (EUCPMTA) with SMTP id C8.5C.08375.12D3BEE5; Thu, 18
 Jun 2020 11:08:33 +0100 (BST)
Received: from [106.109.129.29] (unknown [106.109.129.29]) by
 eusmtip2.samsung.com (KnoxPortal) with ESMTPA id
 20200618100832eusmtip289d70a97d9fe325fcf42c97073b923ff~Zm370hxal2229322293eusmtip2Z;
 Thu, 18 Jun 2020 10:08:32 +0000 (GMT)
To: Ferruh Yigit <ferruh.yigit@intel.com>, dev@dpdk.org,
 v.kuramshin@samsung.com, thomas@monjalon.net, david.marchand@redhat.com,
 arybchenko@solarflare.com, wei.zhao1@intel.com, jia.guo@intel.com,
 beilei.xing@intel.com, qiming.yang@intel.com, wenzhuo.lu@intel.com
From: Ivan Dyukov <i.dyukov@samsung.com>
Message-ID: <5426ad17-a026-7d74-12b8-09516dbd5c98@samsung.com>
Date: Thu, 18 Jun 2020 13:08:22 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.7.0
MIME-Version: 1.0
In-Reply-To: <8d238174-0419-1524-95ad-a1434f848e0d@intel.com>
Content-Transfer-Encoding: 8bit
Content-Language: ru-RU
X-Brightmail-Tracker: H4sIAAAAAAAAA02SfyyUcRzH+z7Pc889jrPHIZ9otW7VquVHWHta6Mf647Ta6j+1XJ3u2bHc
 0R0Kf6SU6ULmZ11lpkQSI5xMjVMOp1B+ZEM2Kj8mhpbzI3X3aPnv9X2/35/t8/7sS+EiA8+V
 ClNFsWqVLFxMCoiaZnOHu9h/Uur1aBUxw1mDJHNn5ibG6Iu1JPNjVo8xA69NfOb78gTOZL2q
 RczscAvBmPpf8pnSn104U92+gg7bShbzC3mSx/XjmKS5P4cvmX7TQ0rSqkqQ5PnoAnmKPCvw
 k7PhYTGs2jPggiA0O7sKj5wKvPquDxJQ2wEtsqGA9oWKe3pSiwSUiC5GYBipRxZDRM8j0NW6
 ccYcgtHEMr4WUdaJhoUgTi9CMNi7THCPaQQpXV/4lmlHOhBypuZwi+FEp2LQ/sTMsxgkvQtM
 yXmYhYV0ACwZ2kgLE/QO0D7LJSzsTAfBasYIwWUcoPX+qJVtaH8Yauq25nF6KyRWP8A5doEZ
 faO1A9BtfKibbSW4csegpKgM49gRJoxVfI43gykzZS0TD78q+/jccDKCoYJba6FDUDX5wdoZ
 p3dDeZ0nJx+BoppSjDuFPXyecuB2sIeMmlyck4WQnCTi0mJoaP24JgOsLNlxsgTGSvV4Otqm
 W1dSt66Ybl0x3f8V8hFRglzYaI1SwWq8VewVD41MqYlWKTwuRigr0d+fZfptnK9FdcshBkRT
 SGwnpBbHpSKeLEYTqzQgoHCxk/Doe5NUJJTLYuNYdcR5dXQ4qzEgN4oQuwh9CsaDRbRCFsVe
 YtlIVv3PxSgb1wTkpTjes9oxXSi9gd2NI4wdAXnxguCnnbddWk56e5YnZO48XeGbFe9MZfd3
 n4n06OzN3DusH4AI4+WHm8wbt5yYMQZi0z6dcSGCcv+D6XVjlZTevGHO7nh2SvV1Hvn1m9zB
 fnXkk1PjtdTtESOCphdp5rd4YE/Auf3ucbZSWz95kpjQhMr27cHVGtkf/a4veFUDAAA=
X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFjrJIsWRmVeSWpSXmKPExsVy+t/xe7qKtq/jDG51K1o8mHKXzaL7QwuT
 xfYVXWwW7z5tZ7K4s/c0u8XzP6+YLabs3MFo8enBCRaL0zc3s1us+XqR2WLrmb+MDtwevxYs
 ZfVYvOclk8exm9PYPd7vu8rm0bdlFaPH6ic/2ALYovRsivJLS1IVMvKLS2yVog0tjPQMLS30
 jEws9QyNzWOtjEyV9O1sUlJzMstSi/TtEvQypk7dwlzw1rPi6HWJBsZTll2MHBwSAiYSB35E
 dDFycQgJLGWUeP/4DjNEXELi9RMgkxPIFJb4c62LDaLmLaNE5+3VYAlhAU+JaW8/M4MkRAR6
 mST6HzexgySEBBYwSSz/XANiswloSJzumMcEYvMK2En8PnSKDcRmEVCV6Fo5nQXEFhWIkPh8
 8AgbRI2gxMmZT8DinAK2EvcOXwGLMwuYSczb/JAZwpaXaN46G8oWl/iw/SDbBEbBWUjaZyFp
 mYWkZRaSlgWMLKsYRVJLi3PTc4sN9YoTc4tL89L1kvNzNzECo3HbsZ+bdzBe2hh8iFGAg1GJ
 hzfhz8s4IdbEsuLK3EOMEhzMSiK8TmdPxwnxpiRWVqUW5ccXleakFh9iNAV6biKzlGhyPjBR
 5JXEG5oamltYGpobmxubWSiJ83YIHIwREkhPLEnNTk0tSC2C6WPi4JRqYPR8FqWywaheSrDZ
 r8a/Vl7MaknV4eDPu7h9J2Taf9z/l0HEJay2f/L9w48vVt296eG73vHeh0s/o+IaLtaeXWvu
 19lauHXStm3/Gf3Tsl8tnmV/6ZTkFX2ulTFrfkYYiCn1swcX9i2Ql33W/MvheIwZ45M1ov9Z
 Jpo4TNv+zLhFdN8dPh7Zr0osxRmJhlrMRcWJALkTSOjcAgAA
X-CMS-MailID: 20200618100833eucas1p15079944aa0fac47bf533d772d12d93e0
X-Msg-Generator: CA
Content-Type: text/plain; charset="utf-8"
X-RootMTR: 20200615090211eucas1p2f9951f582b14d602cbf4d51e228b12a0
X-EPHeader: CA
CMS-TYPE: 201P
X-CMS-RootMailID: 20200615090211eucas1p2f9951f582b14d602cbf4d51e228b12a0
References: <20200427095737.11082-1-i.dyukov@samsung.com>
 <20200615090158.18912-1-i.dyukov@samsung.com>
 <CGME20200615090211eucas1p2f9951f582b14d602cbf4d51e228b12a0@eucas1p2.samsung.com>
 <20200615090158.18912-3-i.dyukov@samsung.com>
 <8d238174-0419-1524-95ad-a1434f848e0d@intel.com>
Subject: Re: [dpdk-dev] [PATCH v3 2/7] 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 <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>

Hi Ferruh,

Thank you for the comments.

My answers are inlined.

Best regards,
Ivan
17.06.2020 19:45, Ferruh Yigit пишет:
> On 6/15/2020 10:01 AM, Ivan Dyukov wrote:
>> This commit add function which treat link status structure
>> and format it to text representation.
> If I am following correctly, the initial need was to escape from speed checks
> everytime loging link information caused by this new 'unknown' speed.
>
> And later suggestion was to have a pre-formatted text for link logging.
Correct.
>
> This patch brings additional link status printing/formatting capability with
> custom format string support and with new format specifiers for link (like, '%D'
> link duplex state),
> although this is nice work and thanks for it, I am not sure this complexity and
> two new APIs are really needed.
Yep.
> For me only 'rte_eth_link_format()' without custom format support looks good
> enough but I won't object if the concensus is to have them.
> I am aware there are multiple applications you are updating logging slightly
> different which requires this flexibility but what happens if they use same
> pre-formatted text, is that difference really required or happened by time based
> on developers taste?
I have changed only few applications but I have plan to change all dpdk 
examples. Even in those few apps, we have various link status logging 
text. e.g  app/test-pmd/config.c
@@ -600,10 +600,9 @@ port_infos_display(portid_t port_id)
         } else
                 printf("\nmemory allocation on the socket: 
%u",port->socket_id);

-       printf("\nLink status: %s\n", (link.link_status) ? ("up") : 
("down"));
-       printf("Link speed: %u Mbps\n", (unsigned) link.link_speed);
-       printf("Link duplex: %s\n", (link.link_duplex == 
ETH_LINK_FULL_DUPLEX) ?
-              ("full-duplex") : ("half-duplex"));
+       rte_eth_link_printf("\nLink status: %S\n"
+                           "Link speed: %M Mbps\n"
+                           "Link duplex: %D\n", &link);
the status is logged in 3 lines. this is special text layoting and I 
don't know how it will look in one line. Myabe it will require reformat 
all screen. I don't want to change screen layoting in this patchset.

> I will put some comments below in any case.
>
>> Signed-off-by: Ivan Dyukov <i.dyukov@samsung.com>
> <...>
>
>> @@ -249,6 +249,9 @@ SRCS-$(CONFIG_RTE_LIBRTE_SECURITY) += test_security.c
>>   
>>   SRCS-$(CONFIG_RTE_LIBRTE_IPSEC) += test_ipsec.c test_ipsec_perf.c
>>   SRCS-$(CONFIG_RTE_LIBRTE_IPSEC) += test_ipsec_sad.c
>> +
>> +SRCS-$(CONFIG_RTE_LIBRTE_ETHER) += test_ethdev_link.c
> +1 to unit test.
>
> <...>
>
>> +int
>> +rte_eth_link_printf(const char *const fmt,
>> +		    struct rte_eth_link *link)
>> +{
>> +	char text[200];
>> +	int ret;
>> +	ret = rte_eth_link_format(text, 200, fmt, link);
> Will it be paranoid to add "text[199] = 0" to be sure any custom 'fmt' won't
> cause any harm?

rte_eth_link_format already do that and it must do that. I would prefer to don't hide rte_eth_link_format bugs in rte_eth_link_printf function.

>
>> +	printf("%s", text);
> Not sure if the error still should be printed on error case?
> Like for example what the code does when fmt="%X"?
yep. I'll add 'ret' check.
>
>> +	return ret;
>> +}
>> +
>> +int
>> +rte_eth_link_format(char *str, int32_t len, const char *const fmt,
>> +		    struct rte_eth_link *link)
> Why not have the 'len' type 'size_t'?

Yes, I can change type of the len but internally we have 'int32_t clen = 
len;'

defined below.  clen should be signed variable because rte_eth_link_format

much more simpler then snprintf. e.g. snprintf may be called with

snprintf(buff, 0, "some text %d", val); no errors returned in this case.

it returns length of formated string.so internally I use signed clen

to detect end of line and avoid uint overflow.

rte_eth_link_format with incorrect or short buffer returns error.

>
>> +	int offset = 0;
>> +	int32_t clen = len;
>> +	const char *fmt_cur = fmt;
>> +	double gbits = (double)link->link_speed / 1000.;
>> +	/* TBD: make it international? */
>> +	static const char LINK_DOWN_STR[]     = "Link down";
>> +	static const char LINK_UP_STR[]       = "Link up at ";
>> +	static const char UNKNOWN_SPEED_STR[] = "Unknown speed";
>> +	static const char MBITS_STR[]	      = "Mbit/s";
>> +	static const char GBITS_STR[]	      = "Gbit/s";
>> +	static const char AUTONEG_STR[]       = "Autoneg";
>> +	static const char FIXED_STR[]         = "Fixed";
>> +	static const char FDX_STR[]           = "FDX";
>> +	static const char HDX_STR[]           = "HDX";
>> +	static const char UNKNOWN_STR[]       = "Unknown";
>> +	static const char UP_STR[]            = "Up";
>> +	static const char DOWN_STR[]          = "Down";
>> +	if (str == NULL || len == 0)
>> +		return -1;
>> +	/* default format string, if no fmt is specified */
>> +	if (fmt == NULL) {
>> +		if (link->link_status == ETH_LINK_DOWN)
>> +			return snprintf(str, (size_t)clen, "%s", LINK_DOWN_STR);
>> +
>> +		offset = snprintf(str, (size_t)clen, "%s", LINK_UP_STR);
>> +		if (offset < 0 || (clen - offset) <= 0)
>> +			return -1;
>> +		clen -= offset;
>> +		str += offset;
>> +		if (link->link_speed == ETH_SPEED_NUM_UNKNOWN) {
>> +			offset = snprintf(str, clen, "%s",
>> +					  UNKNOWN_SPEED_STR);
>> +			if (offset < 0 || (clen - offset) <= 0)
>> +				return -1;
> better to use 'strlcpy' & 'strlcat', they are easier to use for these kind of
> checks.
OK
>
> <...>
>
>> +	/* Formated status */
>> +	} else {
>> +		char c = *fmt_cur;
>> +		while (c) {
>> +			if (clen <= 0)
>> +				return -1;
>> +			if (c == '%') {
>> +				c = *++fmt_cur;
>> +				switch (c) {
>> +				/* Speed in Mbits/s */
>> +				case 'M':
>> +					if (link->link_speed ==
>> +					    ETH_SPEED_NUM_UNKNOWN)
>> +						offset = snprintf(str,
>> +						  clen, "%s",
>> +						  UNKNOWN_STR);
>> +					else
>> +						offset = snprintf(str,
>> +						  clen, "%u",
>> +						  link->link_speed);
> Code readiblity is not great here because you hit the 80char limit, this is a
> sign that something is wrong like function is already too long.
> Can you please try to fix this, like extracting some part of the code to its own
> function or return after end of the 'if' statement which can save one more level
> indentation etc...
agree. I'll define one static function and move part of the code to it. 
It should reduce indent.
>
> <...>
>
>> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
>> index 2090af501..83291e656 100644
>> --- a/lib/librte_ethdev/rte_ethdev.h
>> +++ b/lib/librte_ethdev/rte_ethdev.h
>> @@ -2295,6 +2295,58 @@ int rte_eth_link_get(uint16_t port_id, struct rte_eth_link *link);
>>    */
>>   int rte_eth_link_get_nowait(uint16_t port_id, struct rte_eth_link *link);
>>   
>> +
>> +/**
>> + * print formated link status to stdout. This function threats all
>> + * special values like ETH_SPEED_NUM_UNKNOWN, ETH_LINK_DOWN etc. and convert
>> + * them to textual representation.
>> + *
>> + * @param fmt
>> + *   Format string which allow to format link status. If NULL is provided
>> + *   , default formating will be applied.
>> + *   Following specifiers are available:
>> + *    - '%M' link speed in Mbits/s
>> + *    - '%G' link speed in Gbits/s
>> + *    - '%S' link status. e.g. Up or Down
>> + *    - '%A' link autonegotiation state
>> + *    - '%D' link duplex state
>> + * @param link
>> + *   Link status provided by rte_eth_link_get function
>> + * @return
>> + *   - Number of bytes written to stdout. In case of error, -1 is returned.
> Does it worth to mention the log still will be printed on error?
I'll change the function. It will print nothing on error.
>
>> + *
>> + */
>> +int rte_eth_link_printf(const char *const fmt,
>> +			struct rte_eth_link *link);
>> +
>> +/**
>> + * Format link status to textual representation. This function threats all
>> + * special values like ETH_SPEED_NUM_UNKNOWN, ETH_LINK_DOWN etc. and convert
>> + * them to textual representation.
>> + *
>> + * @param str
>> + *   A pointer to a string to be filled with textual representation of
>> + *   device status.
>> + * @param len
>> + *   Length of available memory at 'str' string.
>> + * @param fmt
>> + *   Format string which allow to format link status. If NULL is provided
>> + *   , default formating will be applied.
>> + *   Following specifiers are available:
>> + *    - '%M' link speed in Mbits/s
>> + *    - '%G' link speed in Gbits/s
>> + *    - '%S' link status. e.g. Up or Down
>> + *    - '%A' link autonegotiation state
>> + *    - '%D' link duplex state
>> + * @param link
>> + *   Link status provided by rte_eth_link_get function
>> + * @return
>> + *   - Number of bytes written to str array. In case of error, -1 is returned.
>> + *
>> + */
>> +int rte_eth_link_format(char *str, int32_t len, const char *const fmt,
>> +			struct rte_eth_link *eth_link);
>> +
> These new APIs needs to be experimental by process (__rte_experimental).
>
> Need the add these APIs to the .map file (rte_ethdev_version.map), so that they
> will be exported in the dynamic library (.so).
>
>