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

[Ivan Dyukov]

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.
>
>> +
>> +			}
>> +			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.