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

[Yigit, Ferruh]

On 7/10/2020 8:02 AM, Ivan Dyukov wrote:
> This commit add function which treat link status structure
> and format it to text representation.
> 
> Signed-off-by: Ivan Dyukov <i.dyukov at samsung.com>

<...>

> +static int
> +rte_eth_link_strf_parser(char *str, size_t len, const char *const fmt,
> +			   const struct rte_eth_link *link)
> +{
> +	size_t offset = 0;
> +	const char *fmt_cur = fmt;
> +	char *str_cur = str;
> +	double gbits = (double)link->link_speed / 1000.;
> +	static const char autoneg_str[]       = "Autoneg";
> +	static const char fixed_str[]         = "Fixed";
> +	static const char fdx_str[]           = "FDX";
> +	static const char hdx_str[]           = "HDX";
> +	static const char unknown_str[]       = "Unknown";
> +	static const char up_str[]            = "Up";
> +	static const char down_str[]          = "Down";
> +	char gbits_str[20];
> +	char mbits_str[20];
> +
> +	/* preformat complex formatting to easily concatinate it further */
> +	snprintf(mbits_str, sizeof(mbits_str), "%u", link->link_speed);
> +	snprintf(gbits_str, sizeof(gbits_str), "%.1f", gbits);
> +	/* init str before formatting */
> +	str[0] = 0;
> +	while (*fmt_cur) {
> +		/* check str bounds */
> +		if (offset > (len - 1)) {
> +			str[len - 1] = '\0';
> +			return -1;
> +		}
> +		if (*fmt_cur == '%') {
> +			/* set null terminator to current position,
> +			 * it's required for strlcat
> +			 */
> +			*str_cur = '\0';
> +			switch (*++fmt_cur) {
> +			/* Speed in Mbits/s */
> +			case 'M':
> +				if (link->link_speed ==
> +				    ETH_SPEED_NUM_UNKNOWN)
> +					offset = strlcat(str, unknown_str,
> +							 len);
> +				else
> +					offset = strlcat(str, mbits_str, len);
> +				break;
> +			/* Speed in Gbits/s */
> +			case 'G':
> +				if (link->link_speed ==
> +				    ETH_SPEED_NUM_UNKNOWN)
> +					offset = strlcat(str, unknown_str,
> +							 len);
> +				else
> +					offset = strlcat(str, gbits_str, len);
> +				break;
> +			/* Link status */
> +			case 'S':
> +				offset = strlcat(str, link->link_status ?
> +					up_str : down_str, len);
> +				break;
> +			/* Link autoneg */
> +			case 'A':
> +				offset = strlcat(str, link->link_autoneg ?
> +					autoneg_str : fixed_str, len);
> +				break;
> +			/* Link duplex */
> +			case 'D':
> +				offset = strlcat(str, link->link_duplex ?
> +					fdx_str : hdx_str, len);
> +				break;
> +			/* ignore unknown specifier */
> +			default:
> +				*str_cur = '%';
> +				offset++;
> +				fmt_cur--;
> +				break;

What do you think ignoring the unknown specifiers and keep continue
processing the string, instead of break? Just keep unknown specifier
as it is in the output string.

> +
> +			}
> +			if (offset > (len - 1))
> +				return -1;

For me "offset >= len" is simpler than "offset > (len - 1)", also it prevents any possible error when "len == 0" ('len' is unsigned) although I can see you have check for it.
Anyway both deos same thing, up to you.

> +
> +			str_cur = str + offset;
> +		} else {
> +			*str_cur++ = *fmt_cur;
> +			offset++;

Why keeping both offset and the pointer ('str_cur'), just offset should be enough "str[offset++] = *fmt_cur;", I think this simplifies a little bit.

> +		}
> +		fmt_cur++;
> +	}
> +	*str_cur = '\0';
> +	return offset;
> +}
> +
> +int
> +rte_eth_link_printf(const char *const fmt,
> +		    const struct rte_eth_link *link)
> +{
> +	char text[200];
> +	int ret;
> +
> +	ret = rte_eth_link_strf(text, 200, fmt, link);
> +	if (ret > 0)
> +		printf("%s", text);
> +	return ret;
> +}
> +
> +int
> +rte_eth_link_strf(char *str, size_t len, const char *const fmt,
> +		    const struct rte_eth_link *link)
> +{
> +	size_t offset = 0;
> +	double gbits = (double)link->link_speed / 1000.;
> +	char speed_gbits_str[20];
> +	char speed_mbits_str[20];
> +	/* TBD: make it international? */
> +	static const char link_down_str[]     = "Link down\n";
> +	static const char link_up_str[]       = "Link up at ";
> +	static const char unknown_speed_str[] = "Unknown speed ";
> +	static const char mbits_str[]	      = "Mbit/s";
> +	static const char gbits_str[]	      = "Gbit/s";
> +	static const char fdx_str[]           = "FDX ";
> +	static const char hdx_str[]           = "HDX ";
> +	/* autoneg is latest param in default string, so add '\n' */
> +	static const char autoneg_str[]       = "Autoneg\n";
> +	static const char fixed_str[]         = "Fixed\n";

Please put an empty line after variables.

<...>

> +/**
> + * Format link status to textual representation. This function threats all
> + * 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.
> + *   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

It should be 'eth_link' otherwise doxygen is complaining.

> + *   Link status provided by rte_eth_link_get function
> + * @return
> + *   - Number of bytes written to str array. In case of error, -1 is returned.
> + *
> + */
> +__rte_experimental
> +int rte_eth_link_strf(char *str, size_t len, const char *const fmt,
> +			const struct rte_eth_link *eth_link);
> +

For the API name, I guess 'strf' is for 'stringify' but what do you think about
'rte_eth_link_to_str()', I think more clear and simpler.