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