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