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.dyu...@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).
>
>
