On Tue, Jun 30, 2020 at 12:24:12PM +0300, Amit Cohen wrote:
> Print extended state in addition to link state.
>
> In case that extended state is not provided, print state only.
> If extended substate is provided in addition to the extended state,
> print it also.
>
> Signed-off-by: Amit Cohen <am...@mellanox.com>
> ---
> netlink/settings.c | 59 +++++++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 56 insertions(+), 3 deletions(-)
>
> diff --git a/netlink/settings.c b/netlink/settings.c
> index 35ba2f5..a4d1908 100644
> --- a/netlink/settings.c
> +++ b/netlink/settings.c
> @@ -604,6 +604,57 @@ int linkinfo_reply_cb(const struct nlmsghdr *nlhdr, void
> *data)
> return MNL_CB_OK;
> }
>
> +int linkstate_link_ext_substate_print(const struct nlattr *tb[],
> + struct nl_context *nlctx, uint8_t
> link_val,
> + uint8_t link_ext_state_val,
> + const char *link_ext_state_str)
> +{
> + uint8_t link_ext_substate_val;
> + const char *link_ext_substate_str;
> +
> + if (!tb[ETHTOOL_A_LINKSTATE_EXT_SUBSTATE])
> + return -ENODATA;
> +
> + link_ext_substate_val =
> mnl_attr_get_u8(tb[ETHTOOL_A_LINKSTATE_EXT_SUBSTATE]);
> +
> + link_ext_substate_str = link_ext_substate_get(link_ext_state_val,
> link_ext_substate_val);
> + if (!link_ext_substate_str)
> + return -ENODATA;
This does not distinguish between missing attribute (substate not
provided by kernel) and unknown value which can happen when older
ethtool version is used on a newer kernel which returns a new substate
not known by ethtool. IMHO we should fall back to reporting the numeric
value in such case rather than behaving as if the information were not
provided.
> +
> + print_banner(nlctx);
> + printf("\tLink detected: %s (%s, %s)\n", link_val ? "yes" : "no",
> + link_ext_state_str, link_ext_substate_str);
> +
> + return 0;
> +}
> +
> +int linkstate_link_ext_state_print(const struct nlattr *tb[],
> + struct nl_context *nlctx, uint8_t link_val)
> +{
> + uint8_t link_ext_state_val;
> + const char *link_ext_state_str;
> + int ret;
> +
> + if (!tb[ETHTOOL_A_LINKSTATE_EXT_STATE])
> + return -ENODATA;
> +
> + link_ext_state_val = mnl_attr_get_u8(tb[ETHTOOL_A_LINKSTATE_EXT_STATE]);
> +
> + link_ext_state_str = link_ext_state_get(link_ext_state_val);
> + if (!link_ext_state_str)
> + return -ENODATA;
The same problem as above.
> +
> + ret = linkstate_link_ext_substate_print(tb, nlctx, link_val,
> link_ext_state_val,
> + link_ext_state_str);
> + if (ret < 0) {
> + print_banner(nlctx);
> + printf("\tLink detected: %s (%s)\n", link_val ? "yes" : "no",
> + link_ext_state_str);
> + }
> +
> + return 0;
> +}
> +
> int linkstate_reply_cb(const struct nlmsghdr *nlhdr, void *data)
> {
> const struct nlattr *tb[ETHTOOL_A_LINKSTATE_MAX + 1] = {};
> @@ -622,9 +673,11 @@ int linkstate_reply_cb(const struct nlmsghdr *nlhdr,
> void *data)
>
> if (tb[ETHTOOL_A_LINKSTATE_LINK]) {
> uint8_t val = mnl_attr_get_u8(tb[ETHTOOL_A_LINKSTATE_LINK]);
> -
> - print_banner(nlctx);
> - printf("\tLink detected: %s\n", val ? "yes" : "no");
> + ret = linkstate_link_ext_state_print(tb, nlctx, val);
> + if (ret < 0) {
> + print_banner(nlctx);
> + printf("\tLink detected: %s\n", val ? "yes" : "no");
> + }
> }
>
> if (tb[ETHTOOL_A_LINKSTATE_SQI]) {
It's a bit impractical to have the banner and first part of the line
printed in three differente places. How about this:
- linkstate_reply_cb() calls print_banner() and prints what it does
now, only without the newline
- linkstate_link_ext_state_print() prints " (%s" or " (%u" with
extended state if it is provided or bails out if not
- linkstate_link_ext_substate_print() prints ", %s" or ", %u" with
extended substate if it is provided
- linkstate_link_ext_state_print() prints ")"
- linkstate_reply_cb() prints the newline
Michal
