[dpdk-dev] [PATCH v9 02/24] ethdev: add a link status text representation

[Ivan Dyukov]

11.08.2020 15:53, Gaëtan Rivet пишет:
> On 11/08/20 15:48 +0300, Ivan Dyukov wrote:
>> 11.08.2020 14:02, Gaëtan Rivet пишет:
>>> On 11/08/20 11:52 +0300, Ivan Dyukov wrote:
>>>> Link status structure keeps complicated values which are hard to
>>>> represent to end user. e.g. link_speed has INT_MAX value which
>>>> means that speed is unknown, link_duplex equal to 0 means
>>>> 'half-duplex' etc. To simplify processing of the values
>>>> in application, new dpdk function is introduced.
>>>>
>>>> This commit adds function which treat link status structure
>>>> and format it to text representation. User may create custom
>>>> link status string using format string. If format string is NULL,
>>>> the function construct standard link status string.
>>>>
>>>> Signed-off-by: Ivan Dyukov <i.dyukov at samsung.com>
>>> Hello Ivan,
>>>
>>> I don't see much difference for this patch, from what I read in previous
>>> thread on the principle it does not seem motivated enough?
>>>
>>> I'd have a few nits on the implementation, but on the principle: I think
>>> this can be an incentive to get properly formatted port status strings.
>>>
>>> The API is a little awkward however, and it is definitely complex to
>>> maintain a format-based function. I think we could do without.
>>>
>>> I've tried a smaller alternative.
>>>
>>>    + simpler to use.
>>>    + simpler to maintain.
>>>    + safer in general.
>>>    + no need to declare local string to store intermediate output.
>>>
>>>    - one ugly macro.
>> It would be good for all values except link_speed because link speed
>> should be formated using sprintf e.g.
>>
>> char str[15];
>>
>> ...
>>
>> char *rte_eth_link_speed_str(uint32_t link_speed) {
>>
>> if (link_speed == UINT32_MAX)
>>
>> return "Unknown";
>>
>> else
>>
>> snprintf(str,sizeof(str),"%d",link_speed);
>>
>> return str;
>>
>> }
>>
>> so rte_eth_link_speed_str will require some global string, not local.
> Sorry I don't understand your point, the implementation below works?
Oh. Sorry I missed it. Thanks for clarification. It should work. Let's 
wait Thomas and Stephen comments.
>>> +/**
>>> + * Missing: doc.
>>> + */
>>> +static inline const char *
>>> +rte_eth_link_speed_str(uint32_t speed)
>>> +{
>>> +       struct {
>>> +               const char *str;
>>> +               uint32_t speed;
>>> +       } speed_str_map[] = {
>>> +               { "Unknown", ETH_SPEED_NUM_NONE },
>>> +               { "10 Mbps", ETH_SPEED_NUM_10M },
>>> +               { "100 Mbps", ETH_SPEED_NUM_100M },
>>> +               { "1 Gbps", ETH_SPEED_NUM_1G },
>>> +               { "2.5 Gbps", ETH_SPEED_NUM_2_5G },
>>> +               { "5 Gbps", ETH_SPEED_NUM_5G },
>>> +               { "10 Gbps", ETH_SPEED_NUM_10G },
>>> +               { "20 Gbps", ETH_SPEED_NUM_20G },
>>> +               { "25 Gbps", ETH_SPEED_NUM_25G },
>>> +               { "40 Gbps", ETH_SPEED_NUM_40G },
>>> +               { "50 Gbps", ETH_SPEED_NUM_50G },
>>> +               { "56 Gbps", ETH_SPEED_NUM_56G },
>>> +               { "100 Gbps", ETH_SPEED_NUM_100G },
>>> +               { "200 Gbps", ETH_SPEED_NUM_200G },
>>> +       };
>>> +       size_t i;
>>> +
>>> +       for (i = 0; i < RTE_DIM(speed_str_map); i++) {
>>> +               if (speed == speed_str_map[i].speed)
>>> +                       return speed_str_map[i].str;
>>> +       }
>>> +
>>> +       return speed_str_map[0].str;
>>> +}