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 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 <dev@dpdk.org>; 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" <ferruh.yigit@intel.com>, "dev@dpdk.org" <dev@dpdk.org>,
"v.kuramshin@samsung.com" <v.kuramshin@samsung.com>, "thomas@monjalon.net"
<thomas@monjalon.net>, "david.marchand@redhat.com"
<david.marchand@redhat.com>, "arybchenko@solarflare.com"
<arybchenko@solarflare.com>, "Zhao1, Wei" <wei.zhao1@intel.com>, "Guo, Jia"
<jia.guo@intel.com>, "Xing, Beilei" <beilei.xing@intel.com>, "Yang, Qiming"
<qiming.yang@intel.com>, "Lu, Wenzhuo" <wenzhuo.lu@intel.com>,
"mb@smartsharesystems.com" <mb@smartsharesystems.com>,
"stephen@networkplumber.org" <stephen@networkplumber.org>, "Chautru,
Nicolas" <nicolas.chautru@intel.com>, "Richardson, Bruce"
<bruce.richardson@intel.com>, "Ananyev, Konstantin"
<konstantin.ananyev@intel.com>, "Dumitrescu, Cristian"
<cristian.dumitrescu@intel.com>, "Nicolau, Radu" <radu.nicolau@intel.com>,
"akhil.goyal@nxp.com" <akhil.goyal@nxp.com>, "Doherty, Declan"
<declan.doherty@intel.com>, "skori@marvell.com" <skori@marvell.com>,
"pbhagavatula@marvell.com" <pbhagavatula@marvell.com>, "jerinj@marvell.com"
<jerinj@marvell.com>, "kirankumark@marvell.com" <kirankumark@marvell.com>,
"Hunt, David" <david.hunt@intel.com>, "Burakov, Anatoly"
<anatoly.burakov@intel.com>, "Li, Xiaoyun" <xiaoyun.li@intel.com>, "Wu,
Jingjing" <jingjing.wu@intel.com>, "Mcnamara, John"
<john.mcnamara@intel.com>, "Singh, Jasvinder" <jasvinder.singh@intel.com>,
"Marohn, Byron" <byron.marohn@intel.com>, "Wang, Yipeng1"
<yipeng1.wang@intel.com>
From: Ivan Dyukov <i.dyukov@samsung.com>
Message-ID: <c4255c4c-e283-6328-b5cd-2d96ef7d2fdf@samsung.com>
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: <fa873f2d21a74ed9960fe0aaf28491da@intel.com>
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>
<CGME20200710070242eucas1p2d638073836aab0f37966a801996ee08b@eucas1p2.samsung.com>
<20200710070226.6045-3-i.dyukov@samsung.com>
<fa873f2d21a74ed9960fe0aaf28491da@intel.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 <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>
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 <i.dyukov@samsung.com>
> <...>
>
>> +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.