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?
> > +/**
> > + * 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;
> > +}
--
Gaëtan
