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 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 ; 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 ; 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 , 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 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> <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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Archived-At: List-Archive: 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 > <...> > >> @@ -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). > >