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

[Ferruh Yigit]

On 6/18/2020 1:32 PM, Morten Brørup wrote:
>> From: dev [mailto:dev-bounces at 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.
> 
>