[PATCH iproute2-net-next 0/3] pretty-print 802.3ad slave state

[PATCH iproute2-net-next 0/3] pretty-print 802.3ad slave state

[Andy Roulin]

Print the bond slave 802.3ad state in a human-readable way in iproute2
The 802.3ad bond slave actor/partner state definitions are exported
to userspace in the kernel include/uapi

rtnetlink sends the bond slave state to userspace, see
 - IFLA_BOND_SLAVE_AD_ACTOR_OPER_PORT_STATE; and
 - IFLA_BOND_SLAVE_AD_PARTNER_OPER_PORT_STATE

These attributes only send the state value; the kernel does not export
the state field definitions to interpret the state's fields.

As the 802.3ad port states are defined in the 802.3ad standard, export
the 802.3ad bond slave state definitions to uapi/ and pretty print
them in iproute2.

net-next:

Andy Roulin (1):
  bonding: move 802.3ad slave state defs to uapi

 drivers/net/bonding/bond_3ad.c  | 10 ----------
 include/uapi/linux/if_bonding.h | 10 ++++++++++
 2 files changed, 10 insertions(+), 10 deletions(-)

iproute2-next:

Andy Roulin (2):
  include/uapi: update bonding kernel header
  iplink: bond: print 3ad actor/partner oper states as strings

 include/uapi/linux/if_bonding.h | 10 +++++++++
 ip/iplink_bond_slave.c          | 38 +++++++++++++++++++++++++++++----
 2 files changed, 44 insertions(+), 4 deletions(-)

-- 
2.20.1

[PATCH net-next 1/3] bonding: move 3ad port state defs to include/uapi

[Andy Roulin]

The actor and partner 802.3ad operating states are exported to userspace
in bond_netlink.c, see bond_slave_fill_info with the following
attributes:

- IFLA_BOND_SLAVE_AD_ACTOR_OPER_PORT_STATE; and
- IFLA_BOND_SLAVE_AD_PARTNER_OPER_PORT_STATE.

The operating states are exported as bitfields and userspace lacks a way
to interpret them, e.g., iproute2 only prints the states as numbers.

For userspace to interpret kernel bitfields, the bitfield definitions
should be part of the uapi. The bitfield itself is defined in the 802.3ad
standard.

This commit moves the 802.3ad bitfield definitions to
uapi/linux/if_bonding.h

Signed-off-by: Andy Roulin <aroulin@cumulusnetworks.com>
Acked-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
Acked-by: Roopa Prabhu <roopa@cumulusnetworks.com>
---
 drivers/net/bonding/bond_3ad.c  | 10 ----------
 include/uapi/linux/if_bonding.h | 10 ++++++++++
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
index 9274dcc6e9b0..503af517bc64 100644
--- a/drivers/net/bonding/bond_3ad.c
+++ b/drivers/net/bonding/bond_3ad.c
@@ -48,16 +48,6 @@
 #define AD_CHURN_DETECTION_TIME    60
 #define AD_AGGREGATE_WAIT_TIME     2
 
-/* Port state definitions (43.4.2.2 in the 802.3ad standard) */
-#define AD_STATE_LACP_ACTIVITY   0x1
-#define AD_STATE_LACP_TIMEOUT    0x2
-#define AD_STATE_AGGREGATION     0x4
-#define AD_STATE_SYNCHRONIZATION 0x8
-#define AD_STATE_COLLECTING      0x10
-#define AD_STATE_DISTRIBUTING    0x20
-#define AD_STATE_DEFAULTED       0x40
-#define AD_STATE_EXPIRED         0x80
-
 /* Port Variables definitions used by the State Machines (43.4.7 in the
  * 802.3ad standard)
  */
diff --git a/include/uapi/linux/if_bonding.h b/include/uapi/linux/if_bonding.h
index 790585f0e61b..6829213a54c5 100644
--- a/include/uapi/linux/if_bonding.h
+++ b/include/uapi/linux/if_bonding.h
@@ -95,6 +95,16 @@
 #define BOND_XMIT_POLICY_ENCAP23	3 /* encapsulated layer 2+3 */
 #define BOND_XMIT_POLICY_ENCAP34	4 /* encapsulated layer 3+4 */
 
+/* 802.3ad port state definitions (43.4.2.2 in the 802.3ad standard) */
+#define AD_STATE_LACP_ACTIVITY   0x1
+#define AD_STATE_LACP_TIMEOUT    0x2
+#define AD_STATE_AGGREGATION     0x4
+#define AD_STATE_SYNCHRONIZATION 0x8
+#define AD_STATE_COLLECTING      0x10
+#define AD_STATE_DISTRIBUTING    0x20
+#define AD_STATE_DEFAULTED       0x40
+#define AD_STATE_EXPIRED         0x80
+
 typedef struct ifbond {
 	__s32 bond_mode;
 	__s32 num_slaves;
-- 
2.20.1

[PATCH iproute2-next 2/3] include/uapi: update bonding kernel header

[Andy Roulin]

The kernel now exports the 802.3ad bond slave state definitions
in uapi. This commit updates the iproute2 bonding uapi to include
these changes.

Signed-off-by: Andy Roulin <aroulin@cumulusnetworks.com>
Acked-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
Acked-by: Roopa Prabhu <roopa@cumulusnetworks.com>
---
 include/uapi/linux/if_bonding.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/include/uapi/linux/if_bonding.h b/include/uapi/linux/if_bonding.h
index 790585f0..6829213a 100644
--- a/include/uapi/linux/if_bonding.h
+++ b/include/uapi/linux/if_bonding.h
@@ -95,6 +95,16 @@
 #define BOND_XMIT_POLICY_ENCAP23	3 /* encapsulated layer 2+3 */
 #define BOND_XMIT_POLICY_ENCAP34	4 /* encapsulated layer 3+4 */
 
+/* 802.3ad port state definitions (43.4.2.2 in the 802.3ad standard) */
+#define AD_STATE_LACP_ACTIVITY   0x1
+#define AD_STATE_LACP_TIMEOUT    0x2
+#define AD_STATE_AGGREGATION     0x4
+#define AD_STATE_SYNCHRONIZATION 0x8
+#define AD_STATE_COLLECTING      0x10
+#define AD_STATE_DISTRIBUTING    0x20
+#define AD_STATE_DEFAULTED       0x40
+#define AD_STATE_EXPIRED         0x80
+
 typedef struct ifbond {
 	__s32 bond_mode;
 	__s32 num_slaves;
-- 
2.20.1

[PATCH iproute2-next 3/3] iplink: bond: print 3ad actor/partner oper states as strings

[Andy Roulin]

The 802.3ad actor/partner operating states are only printed as
numbers, e.g,

ad_actor_oper_port_state 15

Add an additional output in ip link show that prints a string describing
the individual 3ad bit meanings in the following way:

ad_actor_oper_port_state_str <active,short_timeout,aggregating,in_sync>

JSON output is also supported, the field becomes a json array:

"ad_actor_oper_port_state_str":
	["active","short_timeout","aggregating","in_sync"]

Signed-off-by: Andy Roulin <aroulin@cumulusnetworks.com>
Acked-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
Acked-by: Roopa Prabhu <roopa@cumulusnetworks.com>
---
 ip/iplink_bond_slave.c | 38 ++++++++++++++++++++++++++++++++++----
 1 file changed, 34 insertions(+), 4 deletions(-)

diff --git a/ip/iplink_bond_slave.c b/ip/iplink_bond_slave.c
index 4eaf72b8..99beeca1 100644
--- a/ip/iplink_bond_slave.c
+++ b/ip/iplink_bond_slave.c
@@ -68,6 +68,28 @@ static void print_slave_mii_status(FILE *f, struct rtattr *tb)
 			     slave_mii_status[status]);
 }
 
+static void print_slave_oper_state(FILE *fp, const char *name, __u16 state)
+{
+
+	open_json_array(PRINT_ANY, name);
+	if (!is_json_context())
+		fprintf(fp, " <");
+#define _PF(s, str) if (state&AD_STATE_##s) {				\
+			state &= ~AD_STATE_##s;				\
+			print_string(PRINT_ANY, NULL,   		\
+				     state ? "%s," : "%s", str); }
+	_PF(LACP_ACTIVITY, "active");
+	_PF(LACP_TIMEOUT, "short_timeout");
+	_PF(AGGREGATION, "aggregating");
+	_PF(SYNCHRONIZATION, "in_sync");
+	_PF(COLLECTING, "collecting");
+	_PF(DISTRIBUTING, "distributing");
+	_PF(DEFAULTED, "defaulted");
+	_PF(EXPIRED, "expired");
+#undef _PF
+	close_json_array(PRINT_ANY, "> ");
+}
+
 static void bond_slave_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[])
 {
 	SPRINT_BUF(b1);
@@ -106,17 +128,25 @@ static void bond_slave_print_opt(struct link_util *lu, FILE *f, struct rtattr *t
 			  "ad_aggregator_id %d ",
 			  rta_getattr_u16(tb[IFLA_BOND_SLAVE_AD_AGGREGATOR_ID]));
 
-	if (tb[IFLA_BOND_SLAVE_AD_ACTOR_OPER_PORT_STATE])
+	if (tb[IFLA_BOND_SLAVE_AD_ACTOR_OPER_PORT_STATE]) {
+		__u8 state = rta_getattr_u8(tb[IFLA_BOND_SLAVE_AD_ACTOR_OPER_PORT_STATE]);
+
 		print_int(PRINT_ANY,
 			  "ad_actor_oper_port_state",
 			  "ad_actor_oper_port_state %d ",
-			  rta_getattr_u8(tb[IFLA_BOND_SLAVE_AD_ACTOR_OPER_PORT_STATE]));
+			  state);
+		print_slave_oper_state(f, "ad_actor_oper_port_state_str", state);
+	}
+
+	if (tb[IFLA_BOND_SLAVE_AD_PARTNER_OPER_PORT_STATE]) {
+		__u16 state = rta_getattr_u8(tb[IFLA_BOND_SLAVE_AD_PARTNER_OPER_PORT_STATE]);
 
-	if (tb[IFLA_BOND_SLAVE_AD_PARTNER_OPER_PORT_STATE])
 		print_int(PRINT_ANY,
 			  "ad_partner_oper_port_state",
 			  "ad_partner_oper_port_state %d ",
-			  rta_getattr_u16(tb[IFLA_BOND_SLAVE_AD_PARTNER_OPER_PORT_STATE]));
+			  state);
+		print_slave_oper_state(f, "ad_partner_oper_port_state_str", state);
+	}
 }
 
 static int bond_slave_parse_opt(struct link_util *lu, int argc, char **argv,
-- 
2.20.1

Re: [PATCH iproute2-next 3/3] iplink: bond: print 3ad actor/partner oper states as strings

[Nikolay Aleksandrov]

On 27/10/2019 02:29, Andy Roulin wrote:
> The 802.3ad actor/partner operating states are only printed as
> numbers, e.g,
> 
> ad_actor_oper_port_state 15
> 
> Add an additional output in ip link show that prints a string describing
> the individual 3ad bit meanings in the following way:
> 
> ad_actor_oper_port_state_str <active,short_timeout,aggregating,in_sync>
> 
> JSON output is also supported, the field becomes a json array:
> 
> "ad_actor_oper_port_state_str":
> 	["active","short_timeout","aggregating","in_sync"]
> 
> Signed-off-by: Andy Roulin <aroulin@cumulusnetworks.com>
> Acked-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
> Acked-by: Roopa Prabhu <roopa@cumulusnetworks.com>
> ---
>  ip/iplink_bond_slave.c | 38 ++++++++++++++++++++++++++++++++++----
>  1 file changed, 34 insertions(+), 4 deletions(-)
> 
> diff --git a/ip/iplink_bond_slave.c b/ip/iplink_bond_slave.c
> index 4eaf72b8..99beeca1 100644
> --- a/ip/iplink_bond_slave.c
> +++ b/ip/iplink_bond_slave.c
> @@ -68,6 +68,28 @@ static void print_slave_mii_status(FILE *f, struct rtattr *tb)
>  			     slave_mii_status[status]);
>  }
>  
> +static void print_slave_oper_state(FILE *fp, const char *name, __u16 state)
> +{
> +

extra new line here

> +	open_json_array(PRINT_ANY, name);
> +	if (!is_json_context())
> +		fprintf(fp, " <");
> +#define _PF(s, str) if (state&AD_STATE_##s) {				\
> +			state &= ~AD_STATE_##s;				\
> +			print_string(PRINT_ANY, NULL,   		\
> +				     state ? "%s," : "%s", str); }
> +	_PF(LACP_ACTIVITY, "active");
> +	_PF(LACP_TIMEOUT, "short_timeout");
> +	_PF(AGGREGATION, "aggregating");
> +	_PF(SYNCHRONIZATION, "in_sync");
> +	_PF(COLLECTING, "collecting");
> +	_PF(DISTRIBUTING, "distributing");
> +	_PF(DEFAULTED, "defaulted");
> +	_PF(EXPIRED, "expired");
> +#undef _PF
> +	close_json_array(PRINT_ANY, "> ");
> +}
> +
>  static void bond_slave_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[])
>  {
>  	SPRINT_BUF(b1);
> @@ -106,17 +128,25 @@ static void bond_slave_print_opt(struct link_util *lu, FILE *f, struct rtattr *t
>  			  "ad_aggregator_id %d ",
>  			  rta_getattr_u16(tb[IFLA_BOND_SLAVE_AD_AGGREGATOR_ID]));
>  
> -	if (tb[IFLA_BOND_SLAVE_AD_ACTOR_OPER_PORT_STATE])
> +	if (tb[IFLA_BOND_SLAVE_AD_ACTOR_OPER_PORT_STATE]) {
> +		__u8 state = rta_getattr_u8(tb[IFLA_BOND_SLAVE_AD_ACTOR_OPER_PORT_STATE]);
> +
>  		print_int(PRINT_ANY,
>  			  "ad_actor_oper_port_state",
>  			  "ad_actor_oper_port_state %d ",
> -			  rta_getattr_u8(tb[IFLA_BOND_SLAVE_AD_ACTOR_OPER_PORT_STATE]));
> +			  state);
> +		print_slave_oper_state(f, "ad_actor_oper_port_state_str", state);
> +	}
> +
> +	if (tb[IFLA_BOND_SLAVE_AD_PARTNER_OPER_PORT_STATE]) {
> +		__u16 state = rta_getattr_u8(tb[IFLA_BOND_SLAVE_AD_PARTNER_OPER_PORT_STATE]);
>  
> -	if (tb[IFLA_BOND_SLAVE_AD_PARTNER_OPER_PORT_STATE])
>  		print_int(PRINT_ANY,
>  			  "ad_partner_oper_port_state",
>  			  "ad_partner_oper_port_state %d ",
> -			  rta_getattr_u16(tb[IFLA_BOND_SLAVE_AD_PARTNER_OPER_PORT_STATE]));
> +			  state);
> +		print_slave_oper_state(f, "ad_partner_oper_port_state_str", state);
> +	}
>  }
>  
>  static int bond_slave_parse_opt(struct link_util *lu, int argc, char **argv,
> 

Re: [PATCH iproute2-next 3/3] iplink: bond: print 3ad actor/partner oper states as strings

[Stephen Hemminger]

On Sat, 26 Oct 2019 16:29:54 -0700
Andy Roulin <aroulin@cumulusnetworks.com> wrote:

> +	if (!is_json_context())
> +		fprintf(fp, " <");


You don't need conditional here.
Use:
	print_string(PRINT_FP, NULL, " <", NULL);

Re: [PATCH iproute2-net-next 0/3] pretty-print 802.3ad slave state

[David Miller]

Nope, this is not how you do this.

Kernel changes and iproute2 changes must be submitted separately.

I'm tossing all of this, sorry.