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

[Stephen Hemminger]

On Mon,  6 Jul 2020 23:37:16 +0300
Ivan Dyukov <i.dyukov at samsung.com> wrote:

> static int
> +rte_eth_link_strf_parser(char *str, size_t len, const char *const fmt,
> +			   struct rte_eth_link *link)

The link arg should be const.

> +{
> +	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";

Why are these UPPER_CASE_CONSTANTS? That does not match DPDK style.
> +
> +	char gbits_str[20];
> +	char mbits_str[20];
> +	/* preformat complex formatting to easily concatinate it further */

Blank line between declaration and code.
> +	snprintf(mbits_str, 20, "%u", link->link_speed);
> +	snprintf(gbits_str, 20, "%.1f", gbits);

Use sizeof(mbits_str) which is safer and matches what youare doing.
> +	/* 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;
> +			/* Error cases */
> +			default:
> +				return -1;
> +
> +			}
> +			if (offset > (len - 1))
> +				return -1;
> +
> +			str_cur = str + offset;
> +		} else {
> +			*str_cur++ = *fmt_cur;
> +			offset++;
> +		}
> +		fmt_cur++;
> +	}
> +	*str_cur = '\0';
> +	return offset;
> +}
> +
> +int
> +rte_eth_link_printf(const char *const fmt,
> +		    struct rte_eth_link *link)
> +{
> +	char text[200];
> +	int ret;
> +	ret = rte_eth_link_strf(text, 200, fmt, link);

Blank line tween declaration and code.
> +	if (ret > 0)
> +		printf("%s", text);
> +	return ret;
> +}