[dpdk-dev] [PATCH v3 2/7] ethdev: add a link status text representation

[Ivan Dyukov]

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 at 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).
>
>