[PATCH iproute2 net-next] iplink: add support for protodown reason

[PATCH iproute2 net-next] iplink: add support for protodown reason

[Roopa Prabhu]

From: Roopa Prabhu <roopa@cumulusnetworks.com>

This patch adds support for recently
added link IFLA_PROTO_DOWN_REASON attribute.
IFLA_PROTO_DOWN_REASON enumerates reasons
for the already existing IFLA_PROTO_DOWN link
attribute.

$ cat /etc/iproute2/protodown_reasons.d/r.conf
0 mlag
1 evpn
2 vrrp
3 psecurity

$ ip link set dev vx10 protodown on protodown_reason vrrp on
$ip link show dev vx10
14: vx10: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN mode
DEFAULT group default qlen 1000
    link/ether f2:32:28:b8:35:ff brd ff:ff:ff:ff:ff:ff protodown on
protodown_reason <vrrp>
$ip -p -j link show dev vx10
[ {
	<snip>
        "proto_down": true,
        "proto_down_reason": [ "vrrp" ]
} ]
$ip link set dev vx10 protodown_reason mlag on
$ip link show dev vx10
14: vx10: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN mode
DEFAULT group default qlen 1000
    link/ether f2:32:28:b8:35:ff brd ff:ff:ff:ff:ff:ff protodown on
protodown_reason <mlag,vrrp>
$ip -p -j link show dev vx10
[ {
	<snip>
        "proto_down": true,
        "protodown_reason": [ "mlag","vrrp" ]
} ]

$ip -p -j link show dev vx10
$ip link set dev vx10 protodown off protodown_reason vrrp off
Error: Cannot clear protodown, active reasons.
$ip link set dev vx10 protodown off protodown_reason mlag off
$

Note: for somereason the json and non-json key for protodown
are different (protodown and proto_down). I have kept the
same for protodown reason for consistency (protodown_reason and
proto_down_reason).

Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>
---
 include/rt_names.h    |  3 ++
 ip/ipaddress.c        | 45 +++++++++++++++++++---
 ip/iplink.c           | 23 ++++++++++++
 lib/rt_names.c        | 87 +++++++++++++++++++++++++++++++++++++++++++
 man/man8/ip-link.8.in | 10 +++++
 5 files changed, 163 insertions(+), 5 deletions(-)

diff --git a/include/rt_names.h b/include/rt_names.h
index 7afce170..89701a97 100644
--- a/include/rt_names.h
+++ b/include/rt_names.h
@@ -33,6 +33,9 @@ int ll_proto_a2n(unsigned short *id, const char *buf);
 const char *nl_proto_n2a(int id, char *buf, int len);
 int nl_proto_a2n(__u32 *id, const char *arg);
 
+int protodown_reason_a2n(__u32 *id, const char *arg);
+void protodown_reason_n2a(int id, char *buf, int len);
+
 extern int numeric;
 
 #endif
diff --git a/ip/ipaddress.c b/ip/ipaddress.c
index ccf67d1d..1ba7f3f5 100644
--- a/ip/ipaddress.c
+++ b/ip/ipaddress.c
@@ -874,6 +874,44 @@ static void print_link_event(FILE *f, __u32 event)
 	}
 }
 
+static void print_proto_down(FILE *f, struct rtattr *tb[])
+{
+	struct rtattr *preason[IFLA_PROTO_DOWN_REASON_MAX+1];
+
+	if (tb[IFLA_PROTO_DOWN]) {
+		if (rta_getattr_u8(tb[IFLA_PROTO_DOWN]))
+			print_bool(PRINT_ANY,
+				   "proto_down", " protodown on ", true);
+	}
+
+	if (tb[IFLA_PROTO_DOWN_REASON]) {
+		char buf[255];
+		__u32 reason;
+		int i, start = 1;
+
+		parse_rtattr_nested(preason, IFLA_PROTO_DOWN_REASON_MAX,
+				   tb[IFLA_PROTO_DOWN_REASON]);
+		if (!tb[IFLA_PROTO_DOWN_REASON_VALUE])
+			return;
+
+		reason = rta_getattr_u8(preason[IFLA_PROTO_DOWN_REASON_VALUE]);
+		if (!reason)
+			return;
+
+		open_json_array(PRINT_ANY,
+				is_json_context() ? "proto_down_reason" : "protodown_reason <");
+		for (i = 0; reason; i++, reason >>= 1) {
+			if (reason & 0x1) {
+				protodown_reason_n2a(i, buf, sizeof(buf));
+				print_string(PRINT_ANY, NULL,
+					     start ? "%s" : ",%s", buf);
+				start = 0;
+			}
+		}
+		close_json_array(PRINT_ANY, ">");
+	}
+}
+
 int print_linkinfo(struct nlmsghdr *n, void *arg)
 {
 	FILE *fp = (FILE *)arg;
@@ -1066,11 +1104,8 @@ int print_linkinfo(struct nlmsghdr *n, void *arg)
 		print_int(PRINT_FP, NULL, " new-ifindex %d", id);
 	}
 
-	if (tb[IFLA_PROTO_DOWN]) {
-		if (rta_getattr_u8(tb[IFLA_PROTO_DOWN]))
-			print_bool(PRINT_ANY,
-				   "proto_down", " protodown on ", true);
-	}
+	if (tb[IFLA_PROTO_DOWN])
+		print_proto_down(fp, tb);
 
 	if (show_details) {
 		if (tb[IFLA_PROMISCUITY])
diff --git a/ip/iplink.c b/ip/iplink.c
index 7d4b244d..370a802d 100644
--- a/ip/iplink.c
+++ b/ip/iplink.c
@@ -105,6 +105,7 @@ void iplink_usage(void)
 		"			[ nomaster ]\n"
 		"			[ addrgenmode { eui64 | none | stable_secret | random } ]\n"
 		"			[ protodown { on | off } ]\n"
+		"			[ protodown_reason PREASON { on | off } ]\n"
 		"			[ gso_max_size BYTES ] | [ gso_max_segs PACKETS ]\n"
 		"\n"
 		"	ip link show [ DEVICE | group GROUP ] [up] [master DEV] [vrf NAME] [type TYPE]\n"
@@ -903,6 +904,28 @@ int iplink_parse(int argc, char **argv, struct iplink_req *req, char **type)
 				return on_off("protodown", *argv);
 			addattr8(&req->n, sizeof(*req), IFLA_PROTO_DOWN,
 				 proto_down);
+		} else if (strcmp(*argv, "protodown_reason") == 0) {
+			struct rtattr *pr;
+			__u32 preason = 0, prvalue = 0, prmask = 0;
+
+			NEXT_ARG();
+			if (protodown_reason_a2n(&preason, *argv))
+				invarg("invalid protodown reason\n", *argv);
+			NEXT_ARG();
+			prmask = 1 << preason;
+			if (matches(*argv, "on") == 0)
+				prvalue |= prmask;
+			else if (matches(*argv, "off") == 0)
+				prvalue &= ~prmask;
+			else
+				return on_off("protodown_reason", *argv);
+			pr = addattr_nest(&req->n, sizeof(*req),
+					  IFLA_PROTO_DOWN_REASON | NLA_F_NESTED);
+			addattr32(&req->n, sizeof(*req),
+				  IFLA_PROTO_DOWN_REASON_MASK, prmask);
+			addattr32(&req->n, sizeof(*req),
+				  IFLA_PROTO_DOWN_REASON_VALUE, prvalue);
+			addattr_nest_end(&req->n, pr);
 		} else if (strcmp(*argv, "gso_max_size") == 0) {
 			unsigned int max_size;
 
diff --git a/lib/rt_names.c b/lib/rt_names.c
index c40d2e77..cb11c7a6 100644
--- a/lib/rt_names.c
+++ b/lib/rt_names.c
@@ -682,3 +682,90 @@ int nl_proto_a2n(__u32 *id, const char *arg)
 	*id = res;
 	return 0;
 }
+
+#define PROTODOWN_REASON_NUM_BITS 32
+static char *protodown_reason_tab[PROTODOWN_REASON_NUM_BITS] = {
+};
+
+static int protodown_reason_init;
+
+static void protodown_reason_initialize(void)
+{
+	struct dirent *de;
+	DIR *d;
+
+	protodown_reason_init = 1;
+
+	d = opendir(CONFDIR "/protodown_reasons.d");
+	if (!d)
+		return;
+
+	while ((de = readdir(d)) != NULL) {
+		char path[PATH_MAX];
+		size_t len;
+
+		if (*de->d_name == '.')
+			continue;
+
+		/* only consider filenames ending in '.conf' */
+		len = strlen(de->d_name);
+		if (len <= 5)
+			continue;
+		if (strcmp(de->d_name + len - 5, ".conf"))
+			continue;
+
+		snprintf(path, sizeof(path), CONFDIR "/protodown_reasons.d/%s",
+			 de->d_name);
+		rtnl_tab_initialize(path, protodown_reason_tab,
+				    PROTODOWN_REASON_NUM_BITS);
+	}
+	closedir(d);
+}
+
+void protodown_reason_n2a(int id, char *buf, int len)
+{
+	if (id < 0 || id >= PROTODOWN_REASON_NUM_BITS || numeric) {
+		snprintf(buf, len, "%d", id);
+		return;
+	}
+
+	if (!protodown_reason_init)
+		protodown_reason_initialize();
+
+	if (protodown_reason_tab[id])
+		snprintf(buf, len, "%s", protodown_reason_tab[id]);
+	else
+		snprintf(buf, len, "%d", id);
+}
+
+int protodown_reason_a2n(__u32 *id, const char *arg)
+{
+	static char *cache;
+	static unsigned long res;
+	char *end;
+	int i;
+
+	if (cache && strcmp(cache, arg) == 0) {
+		*id = res;
+		return 0;
+	}
+
+	if (!protodown_reason_init)
+		protodown_reason_initialize();
+
+	for (i = 0; i < PROTODOWN_REASON_NUM_BITS; i++) {
+		if (protodown_reason_tab[i] &&
+		    strcmp(protodown_reason_tab[i], arg) == 0) {
+			cache = protodown_reason_tab[i];
+			res = i;
+			*id = res;
+			return 0;
+		}
+	}
+
+	res = strtoul(arg, &end, 0);
+	if (!end || end == arg || *end || res > 255)
+		return -1;
+	*id = res;
+	return 0;
+}
diff --git a/man/man8/ip-link.8.in b/man/man8/ip-link.8.in
index c6bd2c53..df3dd531 100644
--- a/man/man8/ip-link.8.in
+++ b/man/man8/ip-link.8.in
@@ -75,6 +75,9 @@ ip-link \- network device configuration
 .br
 .RB "[ " protodown " { " on " | " off " } ]"
 .br
+.RB "[ " protodown_reason
+.IR PREASON " { " on " | " off " } ]"
+.br
 .RB "[ " trailers " { " on " | " off " } ]"
 .br
 .RB "[ " txqueuelen
@@ -1917,6 +1920,13 @@ state on the device. Indicates that a protocol error has been detected
 on the port. Switch drivers can react to this error by doing a phys
 down on the switch port.
 
+.TP
+.BR "protodown_reason PREASON on " or " off"
+set
+.B PROTODOWN
+reasons on the device. protodown reason bit names can be enumerated under
+/etc/iproute2/protodown_reasons.d/.
+
 .TP
 .BR "dynamic on " or " dynamic off"
 change the
-- 
2.20.1

Re: [PATCH iproute2 net-next] iplink: add support for protodown reason

[Stephen Hemminger]

On Thu, 20 Aug 2020 20:52:02 -0700
Roopa Prabhu <roopa@cumulusnetworks.com> wrote:

> +	if (tb[IFLA_PROTO_DOWN]) {
> +		if (rta_getattr_u8(tb[IFLA_PROTO_DOWN]))
> +			print_bool(PRINT_ANY,
> +				   "proto_down", " protodown on ", true);

In general my preference is to use print_null() for presence flags.
Otherwise you have to handle the false case in JSON as a special case.

Re: [PATCH iproute2 net-next] iplink: add support for protodown reason

[Roopa Prabhu]

On 8/20/20 9:36 PM, Stephen Hemminger wrote:
>
>
> On Thu, 20 Aug 2020 20:52:02 -0700
> Roopa Prabhu <roopa@cumulusnetworks.com> wrote:
>
>> +     if (tb[IFLA_PROTO_DOWN]) {
>> +             if (rta_getattr_u8(tb[IFLA_PROTO_DOWN]))
>> +                     print_bool(PRINT_ANY,
>> +                                "proto_down", " protodown on ", true);
> In general my preference is to use print_null() for presence flags.
> Otherwise you have to handle the false case in JSON as a special case.


ok, i will look. But this is existing code moved into a new function and 
has been

working fine for years.

Re: [PATCH iproute2 net-next] iplink: add support for protodown reason

[Roopa Prabhu]

On 8/20/20 10:18 PM, Roopa Prabhu wrote:
>
> On 8/20/20 9:36 PM, Stephen Hemminger wrote:
>>
>>
>> On Thu, 20 Aug 2020 20:52:02 -0700
>> Roopa Prabhu <roopa@cumulusnetworks.com> wrote:
>>
>>> +     if (tb[IFLA_PROTO_DOWN]) {
>>> +             if (rta_getattr_u8(tb[IFLA_PROTO_DOWN]))
>>> +                     print_bool(PRINT_ANY,
>>> +                                "proto_down", " protodown on ", true);
>> In general my preference is to use print_null() for presence flags.
>> Otherwise you have to handle the false case in JSON as a special case.
>
>
> ok, i will look. But this is existing code moved into a new function and
> has been
>
> working fine for years.


looked at print_null and switching to that results in a change in output 
for existing protodown

attribute, so I plan to leave it as is for now.

Re: [PATCH iproute2 net-next] iplink: add support for protodown reason

[Stephen Hemminger]

On Fri, 21 Aug 2020 14:09:14 -0700
Roopa Prabhu <roopa@nvidia.com> wrote:

> On 8/20/20 10:18 PM, Roopa Prabhu wrote:
> >
> > On 8/20/20 9:36 PM, Stephen Hemminger wrote:  
> >>
> >>
> >> On Thu, 20 Aug 2020 20:52:02 -0700
> >> Roopa Prabhu <roopa@cumulusnetworks.com> wrote:
> >>  
> >>> +     if (tb[IFLA_PROTO_DOWN]) {
> >>> +             if (rta_getattr_u8(tb[IFLA_PROTO_DOWN]))
> >>> +                     print_bool(PRINT_ANY,
> >>> +                                "proto_down", " protodown on ", true);  
> >> In general my preference is to use print_null() for presence flags.
> >> Otherwise you have to handle the false case in JSON as a special case.  
> >
> >
> > ok, i will look. But this is existing code moved into a new function and
> > has been
> >
> > working fine for years.  
> 
> 
> looked at print_null and switching to that results in a change in output 
> for existing protodown
> 
> attribute, so I plan to leave it as is for now.
> 

Sure we should really try and have some consistency in the JSON output.
Maybe a JSON style guide is needed, I wonder if some heavy JSON user already
has one?

Re: [PATCH iproute2 net-next] iplink: add support for protodown reason

[David Ahern]

On 8/20/20 9:52 PM, Roopa Prabhu wrote:
> +void protodown_reason_n2a(int id, char *buf, int len)
> +{
> +	if (id < 0 || id >= PROTODOWN_REASON_NUM_BITS || numeric) {

since the reason is limited to 0-31, id > PROTODOWN_REASON_NUM_BITS
should be an error.

> +		snprintf(buf, len, "%d", id);
> +		return;
> +	}
> +
> +	if (!protodown_reason_init)
> +		protodown_reason_initialize();
> +
> +	if (protodown_reason_tab[id])
> +		snprintf(buf, len, "%s", protodown_reason_tab[id]);
> +	else
> +		snprintf(buf, len, "%d", id);
> +}
> +
> +int protodown_reason_a2n(__u32 *id, const char *arg)
> +{
> +	static char *cache;
> +	static unsigned long res;
> +	char *end;
> +	int i;
> +
> +	if (cache && strcmp(cache, arg) == 0) {
> +		*id = res;
> +		return 0;
> +	}
> +
> +	if (!protodown_reason_init)
> +		protodown_reason_initialize();
> +
> +	for (i = 0; i < PROTODOWN_REASON_NUM_BITS; i++) {
> +		if (protodown_reason_tab[i] &&
> +		    strcmp(protodown_reason_tab[i], arg) == 0) {
> +			cache = protodown_reason_tab[i];
> +			res = i;
> +			*id = res;
> +			return 0;
> +		}
> +	}
> +
> +	res = strtoul(arg, &end, 0);
> +	if (!end || end == arg || *end || res > 255)

same here: res >= PROTODOWN_REASON_NUM_BITS is a failure.

> +		return -1;
> +	*id = res;
> +	return 0;
> +}
> diff --git a/man/man8/ip-link.8.in b/man/man8/ip-link.8.in
> index c6bd2c53..df3dd531 100644
> --- a/man/man8/ip-link.8.in
> +++ b/man/man8/ip-link.8.in
> @@ -75,6 +75,9 @@ ip-link \- network device configuration
>  .br
>  .RB "[ " protodown " { " on " | " off " } ]"
>  .br
> +.RB "[ " protodown_reason
> +.IR PREASON " { " on " | " off " } ]"
> +.br
>  .RB "[ " trailers " { " on " | " off " } ]"
>  .br
>  .RB "[ " txqueuelen
> @@ -1917,6 +1920,13 @@ state on the device. Indicates that a protocol error has been detected
>  on the port. Switch drivers can react to this error by doing a phys
>  down on the switch port.
>  
> +.TP
> +.BR "protodown_reason PREASON on " or " off"
> +set
> +.B PROTODOWN
> +reasons on the device. protodown reason bit names can be enumerated under
> +/etc/iproute2/protodown_reasons.d/.

we should document that 0..31 limit.

> +
>  .TP
>  .BR "dynamic on " or " dynamic off"
>  change the
> 

I wonder how well supported __builtin_ffsl is across architectures ...
would be faster than the 'for (i = 0; reason; i++, reason >>= 1) {' checks.

Re: [PATCH iproute2 net-next] iplink: add support for protodown reason

[Roopa Prabhu]

On 8/21/20 6:30 PM, Stephen Hemminger wrote:
> External email: Use caution opening links or attachments
>
>
> On Fri, 21 Aug 2020 14:09:14 -0700
> Roopa Prabhu <roopa@nvidia.com> wrote:
>
>> On 8/20/20 10:18 PM, Roopa Prabhu wrote:
>>> On 8/20/20 9:36 PM, Stephen Hemminger wrote:
>>>>
>>>> On Thu, 20 Aug 2020 20:52:02 -0700
>>>> Roopa Prabhu <roopa@cumulusnetworks.com> wrote:
>>>>
>>>>> +     if (tb[IFLA_PROTO_DOWN]) {
>>>>> +             if (rta_getattr_u8(tb[IFLA_PROTO_DOWN]))
>>>>> +                     print_bool(PRINT_ANY,
>>>>> +                                "proto_down", " protodown on ", true);
>>>> In general my preference is to use print_null() for presence flags.
>>>> Otherwise you have to handle the false case in JSON as a special case.
>>>
>>> ok, i will look. But this is existing code moved into a new function and
>>> has been
>>>
>>> working fine for years.
>>
>> looked at print_null and switching to that results in a change in output
>> for existing protodown
>>
>> attribute, so I plan to leave it as is for now.
>>
> Sure we should really try and have some consistency in the JSON output.
> Maybe a JSON style guide is needed, I wonder if some heavy JSON user already
> has one?

yes, agreed. I think we need to checkin a guide for coders and 
reviewers. specifically for the iproute2 json library.

Its hard to catch these at review time otherwise and most of iproute2 
bugs are propagated due to copy-paste.

I checked if the FRR project had one as they have a lot of json routing 
operational data. They don't and  rely on reviewers.

In the least i think the json library api documentation and a few best 
practices will help.

thanks for the review.

Re: [PATCH iproute2 net-next] iplink: add support for protodown reason

[Roopa Prabhu]

On 8/23/20 7:20 PM, David Ahern wrote:
> External email: Use caution opening links or attachments
>
>
> On 8/20/20 9:52 PM, Roopa Prabhu wrote:
>> +void protodown_reason_n2a(int id, char *buf, int len)
>> +{
>> +     if (id < 0 || id >= PROTODOWN_REASON_NUM_BITS || numeric) {
> since the reason is limited to 0-31, id > PROTODOWN_REASON_NUM_BITS
> should be an error.


copy paste from other functions in rt_names which do the same thing.

v2 coming with fix for this and other comments. thanks for the review.