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 51420A052A; Fri, 10 Jul 2020 20:36:59 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 47C3A1DC97; Fri, 10 Jul 2020 20:36:58 +0200 (CEST) Received: from mailout2.w1.samsung.com (mailout2.w1.samsung.com [210.118.77.12]) by dpdk.org (Postfix) with ESMTP id 49A6B1DC77 for ; Fri, 10 Jul 2020 20:36:57 +0200 (CEST) Received: from eucas1p2.samsung.com (unknown [182.198.249.207]) by mailout2.w1.samsung.com (KnoxPortal) with ESMTP id 20200710183656euoutp02ae0111835eb996271b4875e0bc612dea~geAGsh23e1614216142euoutp02y; Fri, 10 Jul 2020 18:36:56 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 mailout2.w1.samsung.com 20200710183656euoutp02ae0111835eb996271b4875e0bc612dea~geAGsh23e1614216142euoutp02y DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=samsung.com; s=mail20170921; t=1594406216; bh=4WZ9AKL4FnvE53ZElZl0VP+sNWqmmNxb2mPRva8KFls=; h=Subject:To:From:Date:In-Reply-To:References:From; b=Ikbmw6jHpc4SRmBOAA77gRVuLRYKWMeZdVCWCQ8+XFbpB7vsrEEN7Fct+nwWMxnVD NLZkUUtla/xZJECo3RgwSEmqnYy41d1aPUM6Z76ev8ErFpZvzh5OVWK0didM8PZHgH ILRI8fWI+4Sz5dgwBt7BPqzzKKMrozZnxCR1B6yI= Received: from eusmges1new.samsung.com (unknown [203.254.199.242]) by eucas1p2.samsung.com (KnoxPortal) with ESMTP id 20200710183656eucas1p2f22652a7904b5722ce2c411dffe2ee86~geAGFcY7D1435514355eucas1p23; Fri, 10 Jul 2020 18:36:56 +0000 (GMT) Received: from eucas1p1.samsung.com ( [182.198.249.206]) by eusmges1new.samsung.com (EUCPMTA) with SMTP id ED.9C.06456.745B80F5; Fri, 10 Jul 2020 19:36:55 +0100 (BST) Received: from eusmtrp1.samsung.com (unknown [182.198.249.138]) by eucas1p1.samsung.com (KnoxPortal) with ESMTPA id 20200710183654eucas1p1a50b6f4b5a6e4404ad2cd1617c4f1692~geAE_A4p42963829638eucas1p1w; Fri, 10 Jul 2020 18:36:54 +0000 (GMT) Received: from eusmgms2.samsung.com (unknown [182.198.249.180]) by eusmtrp1.samsung.com (KnoxPortal) with ESMTP id 20200710183654eusmtrp10f441acf81775ae90f773d32760d2adc~geAE8-4t21272612726eusmtrp1Q; Fri, 10 Jul 2020 18:36:54 +0000 (GMT) X-AuditID: cbfec7f2-7efff70000001938-67-5f08b547f0db Received: from eusmtip1.samsung.com ( [203.254.199.221]) by eusmgms2.samsung.com (EUCPMTA) with SMTP id FC.BF.06017.645B80F5; Fri, 10 Jul 2020 19:36:54 +0100 (BST) Received: from [106.109.129.29] (unknown [106.109.129.29]) by eusmtip1.samsung.com (KnoxPortal) with ESMTPA id 20200710183652eusmtip17bc16ce4aeca23130303bc64252c6834~geADInClP1749417494eusmtip1g; Fri, 10 Jul 2020 18:36:52 +0000 (GMT) To: "Yigit, Ferruh" , "dev@dpdk.org" , "v.kuramshin@samsung.com" , "thomas@monjalon.net" , "david.marchand@redhat.com" , "arybchenko@solarflare.com" , "Zhao1, Wei" , "Guo, Jia" , "Xing, Beilei" , "Yang, Qiming" , "Lu, Wenzhuo" , "mb@smartsharesystems.com" , "stephen@networkplumber.org" , "Chautru, Nicolas" , "Richardson, Bruce" , "Ananyev, Konstantin" , "Dumitrescu, Cristian" , "Nicolau, Radu" , "akhil.goyal@nxp.com" , "Doherty, Declan" , "skori@marvell.com" , "pbhagavatula@marvell.com" , "jerinj@marvell.com" , "kirankumark@marvell.com" , "Hunt, David" , "Burakov, Anatoly" , "Li, Xiaoyun" , "Wu, Jingjing" , "Mcnamara, John" , "Singh, Jasvinder" , "Marohn, Byron" , "Wang, Yipeng1" From: Ivan Dyukov Message-ID: Date: Fri, 10 Jul 2020 21:36:51 +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: Content-Transfer-Encoding: 8bit Content-Language: ru-RU X-Brightmail-Tracker: H4sIAAAAAAAAA02Se0xTZxjG851zenroWnMsGt7gIrNqZCbimMa8ZkY2w+JJ/GNoFuOdVTkB Ii2mFbXOGKKzw4KI5eIEgoxuQYENhUIRAQUXLiuXwHCTCdIwCJFradV5d7QHM/77vc/zfPme N3k5Wt3ABnPx+qOiQa9N0LAKprr5ReearVVc9CftP32G5e0FBIce2Wh0ZQ2wmOr+jsIHJRFY 58iXoXUwmcJ06wyFjmsWFidcZxic8jgo7K93ytHTNCrHC115BEdfj9F4pdBLsKugSoYvGmtZ nH5dxGD6y/sEL9pbCbakzTCYdauGoNnCo/t6sRxt/UvR42pl0NlXKceyp900VrW/IXippZxg h/md7POPhJeFP8sEW91jSsj8sZsWmvty5EJaWwEl3JyqoYTphvuskG4vIcLvQ6m0UDr8nI1S 7FFsihET4o+JhrWbv1HEeSer6SP3wk+M3BmQJZOJFRYSwAG/HvLrMmUWouDU/DUCz/94Njc8 IXDbc5aRBi+Bkd7vZe+f/OLuYiWjmEBe2RQtDdMEhs9bGF8qkN8GngttlM9YxLs+gB+8vX6D 5UPBmVJA+VjFb4aGKZefGX4l3KmbYX28mN8F76z/MFJmIbRdGfZzAL8RzlWP+fM0HwJnq/Jo iYPA7WhkpXpPOBiwHZA4ErI7ximJA2GsxS6X+ENwZqYxEn8L/1b8JfcVBT6FwKOic3OhCLCP d84yN/vBx1Beu1aSv4Bic71fBn4BPJhcKFVYANbqy7QkqyDFrJbSGrjb1jMnA7x5pZRkAUYu lZIMsix33o658/bKnbdX7v8VCglTQoLEJKMuVjSG68XjYUatzpikjw07lKirILNX7Xzb4qkh T3sONhGeIxqlqlDLRatl2mNGk66JAEdrFqm2dDgPqFUxWtNJ0ZAYbUhKEI1NZAnHaIJU64oe 71fzsdqj4mFRPCIa3rsUFxCcTI6HnVeefhv1VeTDv/t+DdiwwWboVYZW7N1d3/h1a8/ynV+u 8OhuePZts9MRg/G4fdl4Vmbn0K71a1ZV0g+9V2MG+ZWmezusMVsGKwcuPjuRfVO59dTO0iz9 bVNGVO2no4kTlslVIRqTvbj/z8tx2Y7+dTkZ3lPu+jxz4J7g31LHO+/maBhjnDZ8NW0wav8D E2yRgdEDAAA= X-Brightmail-Tracker: H4sIAAAAAAAAA02SSUwTYRiG889MpwNaHQrEPxzUNBC3UCyL/aqIW6ITvZDIwaiII0zASFuc aY16kbggVkHBJaGapkpwQY3BAgWskFZlq4ZwQCWKVsUgqFjADYJiC5hwe76873P4kpchlU9k Ucxeg0kQDXyuig6lvH9bemI31jAZy5vKl8G9pzYE79+Uk+C70EPDaf9xAl5WrgGX84oMSt/m E1BcOkSA86aFhi++oxQMDjsJeP3QK4dhT58cijouI+gbHyChzD6CoMNWI4NRdwMN38avUVA8 1oXgbHUrgpYzQxRcqK9DUGBhwX/rhhzKX8+HYV8rBd5uhxzu/OgkoebpHwQlLfcQPCuYkK1d yI3ZK2Rcuauf4M5f7SS55u5Lcu5Mm43gqgbrCO5bYxfNFVdXIq79/WmSu937m04N3a5OFo1m k7AwxyiZVqt2aCBerdGBOj5Rp9YkaNNXxiep4lKSs4TcvQcEMS5ltzpn5GstmfdIc/BjU48s H32JtqAQBrOJ+K6/g7agUEbJViB8/UNZ4GACAcafe8mpTjgef26Z7nxF+KerGAWDcHYLHi5q I4JBBDswCx9/2y8LBkrWSmDfxK4g0+xi7C20EUFWsCm4cdA3yRQbg5tcQ3SQI9lteMT9mJ7q hOG2sl4qyCGsDp+oHZjsk+wKbHO8I6d4AT5Wc3ma52G/002fQ2HWGbp1hmKdoVhnKHZEVaII wSzps/VSvFri9ZLZkK3ONOrvo8CEaptHq+uQZXCrB7EMUs1W2HkmQynjD0iH9B6EGVIVoVj/ zLtLqcjiDx0WRGOGaM4VJA9KCjxXQkZFZhoDgzSYMjRJGi3oNNoEbcIKUM1TFLLunUo2mzcJ +wQhTxD/ewQTEpWPdCs3Rzc0pI1dkf/opmL2+cdeOHo3bHm+e86q9j230PK073nhn9zehCUo lTAUzV6U7nd5Yuv7G+OcdRXt89OqqrUX1+1vSiz6JYQ5xQcPRmP8lZvoJ/a5pw5q+461npzr f6ils/htaldzVeQHicnMF81HHOk7L+6xjTvPJTteJaooKYfXLCVFif8Hd1RZyVgDAAA= X-CMS-MailID: 20200710183654eucas1p1a50b6f4b5a6e4404ad2cd1617c4f1692 X-Msg-Generator: CA Content-Type: text/plain; charset="utf-8" X-RootMTR: 20200710070242eucas1p2d638073836aab0f37966a801996ee08b X-EPHeader: CA CMS-TYPE: 201P X-CMS-RootMailID: 20200710070242eucas1p2d638073836aab0f37966a801996ee08b References: <20200427095737.11082-1-i.dyukov@samsung.com> <20200710070226.6045-1-i.dyukov@samsung.com> <20200710070226.6045-3-i.dyukov@samsung.com> Subject: Re: [dpdk-dev] [PATCH v7 02/25] 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" 10.07.2020 16:06, Yigit, Ferruh пишет: > On 7/10/2020 8:02 AM, Ivan Dyukov wrote: >> This commit add function which treat link status structure >> and format it to text representation. >> >> Signed-off-by: Ivan Dyukov > <...> > >> +static int >> +rte_eth_link_strf_parser(char *str, size_t len, const char *const fmt, >> + const struct rte_eth_link *link) >> +{ >> + size_t offset = 0; >> + const char *fmt_cur = fmt; >> + char *str_cur = str; >> + double gbits = (double)link->link_speed / 1000.; >> + 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"; >> + char gbits_str[20]; >> + char mbits_str[20]; >> + >> + /* preformat complex formatting to easily concatinate it further */ >> + snprintf(mbits_str, sizeof(mbits_str), "%u", link->link_speed); >> + snprintf(gbits_str, sizeof(gbits_str), "%.1f", gbits); >> + /* init str before formatting */ >> + str[0] = 0; >> + while (*fmt_cur) { >> + /* check str bounds */ >> + if (offset > (len - 1)) { >> + str[len - 1] = '\0'; >> + return -1; >> + } >> + if (*fmt_cur == '%') { >> + /* set null terminator to current position, >> + * it's required for strlcat >> + */ >> + *str_cur = '\0'; >> + switch (*++fmt_cur) { >> + /* Speed in Mbits/s */ >> + case 'M': >> + if (link->link_speed == >> + ETH_SPEED_NUM_UNKNOWN) >> + offset = strlcat(str, unknown_str, >> + len); >> + else >> + offset = strlcat(str, mbits_str, len); >> + break; >> + /* Speed in Gbits/s */ >> + case 'G': >> + if (link->link_speed == >> + ETH_SPEED_NUM_UNKNOWN) >> + offset = strlcat(str, unknown_str, >> + len); >> + else >> + offset = strlcat(str, gbits_str, len); >> + break; >> + /* Link status */ >> + case 'S': >> + offset = strlcat(str, link->link_status ? >> + up_str : down_str, len); >> + break; >> + /* Link autoneg */ >> + case 'A': >> + offset = strlcat(str, link->link_autoneg ? >> + autoneg_str : fixed_str, len); >> + break; >> + /* Link duplex */ >> + case 'D': >> + offset = strlcat(str, link->link_duplex ? >> + fdx_str : hdx_str, len); >> + break; >> + /* ignore unknown specifier */ >> + default: >> + *str_cur = '%'; >> + offset++; >> + fmt_cur--; >> + break; > What do you think ignoring the unknown specifiers and keep continue > processing the string, instead of break? Just keep unknown specifier > as it is in the output string. yep. it's exactly what code do.  break exit from the switch but not from string processing.  I have unit tests for this case. They  work fine. Please review unit tests and send me more cases if they need to be tested. > >> + >> + } >> + if (offset > (len - 1)) >> + return -1; > For me "offset >= len" is simpler than "offset > (len - 1)", also it prevents any possible error when "len == 0" ('len' is unsigned) although I can see you have check for it. > Anyway both deos same thing, up to you. > >> + >> + str_cur = str + offset; >> + } else { >> + *str_cur++ = *fmt_cur; >> + offset++; > Why keeping both offset and the pointer ('str_cur'), just offset should be enough "str[offset++] = *fmt_cur;", I think this simplifies a little bit. > >> + } >> + fmt_cur++; >> + } >> + *str_cur = '\0'; >> + return offset; >> +} >> + >> +int >> +rte_eth_link_printf(const char *const fmt, >> + const struct rte_eth_link *link) >> +{ >> + char text[200]; >> + int ret; >> + >> + ret = rte_eth_link_strf(text, 200, fmt, link); >> + if (ret > 0) >> + printf("%s", text); >> + return ret; >> +} >> + >> +int >> +rte_eth_link_strf(char *str, size_t len, const char *const fmt, >> + const struct rte_eth_link *link) >> +{ >> + size_t offset = 0; >> + double gbits = (double)link->link_speed / 1000.; >> + char speed_gbits_str[20]; >> + char speed_mbits_str[20]; >> + /* TBD: make it international? */ >> + static const char link_down_str[] = "Link down\n"; >> + 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 fdx_str[] = "FDX "; >> + static const char hdx_str[] = "HDX "; >> + /* autoneg is latest param in default string, so add '\n' */ >> + static const char autoneg_str[] = "Autoneg\n"; >> + static const char fixed_str[] = "Fixed\n"; > Please put an empty line after variables. > > <...> > >> +/** >> + * 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 formatting 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 > It should be 'eth_link' otherwise doxygen is complaining. > >> + * Link status provided by rte_eth_link_get function >> + * @return >> + * - Number of bytes written to str array. In case of error, -1 is returned. >> + * >> + */ >> +__rte_experimental >> +int rte_eth_link_strf(char *str, size_t len, const char *const fmt, >> + const struct rte_eth_link *eth_link); >> + > For the API name, I guess 'strf' is for 'stringify' but what do you think about > 'rte_eth_link_to_str()', I think more clear and simpler.