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

[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?

> > +/**
> > + * 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