Re: [dpdk-dev] [PATCH v7 02/25] ethdev: add a link status text representation

[Thomas Monjalon <thomas@monjalon.net>]

10/07/2020 09:02, Ivan Dyukov:
> This commit add function which treat link status structure
> and format it to text representation.

It is missing an explanation about why it is required.
Which problem is it solving?

> Signed-off-by: Ivan Dyukov <i.dyukov@samsung.com>

I'm surprised there is not so much review on this patch.

[...]
> --- a/lib/librte_ethdev/rte_ethdev.h
> +++ b/lib/librte_ethdev/rte_ethdev.h
> +/**
> + * print formatted link status to stdout. This function threats all
> + * special values like ETH_SPEED_NUM_UNKNOWN, ETH_LINK_DOWN etc. and convert
> + * them to textual representation.

I don't understand the need for this function.
If needed, the application can send the output of rte_eth_link_strf()
to fprintf or a log function.

> + *
> + * @param fmt
> + *   Format string which allow to format link status. If NULL is provided
> + *   , default formatting will be applied.
> + *   Following specifiers are available:
> + *    - '%M' link speed in Mbits/s
> + *    - '%G' link speed in Gbits/s
> + *    - '%S' link status. e.g. Up or Down
> + *    - '%A' link autonegotiation state
> + *    - '%D' link duplex state
> + * @param link
> + *   Link status provided by rte_eth_link_get function
> + * @return
> + *   - Number of bytes written to stdout. In case of error, -1 is returned.
> + *
> + */
> +__rte_experimental
> +int rte_eth_link_printf(const char *const fmt,
> +			const struct rte_eth_link *link);
> +
> +/**
> + * Format link status to textual representation. This function threats all

not "threats"

> + * special values like ETH_SPEED_NUM_UNKNOWN, ETH_LINK_DOWN etc. and convert
> + * them to textual representation.
> + *
> + * @param str
> + *   A pointer to a string to be filled with textual representation of
> + *   device status.
> + * @param len
> + *   Length of available memory at 'str' string.
> + * @param fmt
> + *   Format string which allow to format link status. If NULL is provided
> + *   , default formatting will be applied.

Please do not start a line with a comma,
it should be ending the previous line, like here ;)
Even better: start the new sentence on a new line.

> + *   Following specifiers are available:
> + *    - '%M' link speed in Mbits/s
> + *    - '%G' link speed in Gbits/s
> + *    - '%S' link status. e.g. Up or Down
> + *    - '%A' link autonegotiation state
> + *    - '%D' link duplex state

These specifiers look OK.

> + * @param link
> + *   Link status provided by rte_eth_link_get function
> + * @return
> + *   - Number of bytes written to str array. In case of error, -1 is returned.

Better to have error case on a separate line.

> + *
> + */
> +__rte_experimental
> +int rte_eth_link_strf(char *str, size_t len, const char *const fmt,
> +			const struct rte_eth_link *eth_link);