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.dyu...@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;
>>> +}
