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

[Yigit, Ferruh]

> Subject: Re: [PATCH v7 02/25] ethdev: add a link status text representation
> 
> 10.07.2020 16:06, 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.
> yep. it's exactly what code do.  break exit from the switch but not from string
> processing.  I have unit tests for this case. They  work fine.
> Please review unit tests and send me more cases if they need to be tested.

yes it does as you explained, I miss read it, so it is good, please ignore this comment.