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.dyu...@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.
