On 6/18/2020 1:32 PM, Morten Brørup wrote: >> From: dev [mailto:dev-boun...@dpdk.org] On Behalf Of Ferruh Yigit >> Sent: Thursday, June 18, 2020 2:03 PM >> >> On 6/18/2020 11:08 AM, Ivan Dyukov wrote: >>> 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. >> >> cc'ed Morten & Stephen. >> >> To keep existing layout in the applications yes you need more >> flexibility, a >> pre-formatted text won't cut it, but I guess I prefer your approach in >> v2 for >> simplicity. >> >> And I would prefer extending the one in v2 with a larger string for the >> pre-formatted text, so both flexibility and the standard output can be >> provided, >> Morten didn't like it but if I understand correctly his comment was to >> keep more >> simple solution in ethdev and applications can do it themselves if they >> want >> more custom log formatting, but this patch adds more complex and >> flexible >> logging support to the ethdev. >> > > If you are going to add a flexible link status formatting function, like > strftime(), it should not print to stdout, but to a string buffer, like > strftime(). It will also allow for new format specifiers in the future, e.g. > %1D for "FDX"/"HDX", %2D for "Full"/"Half" or %3D for > "full-duplex"/"half-duplex". This would provide more flexible support for a > larger number of application specific formats.
These will make it even more complex, do we really need this? > > Then the function for printing to stdout in a DPDK preferred format (if you > add such a function to the library) can use the above eth_link_strf() > function. > >
