bridge: Add backup nexthop ID support

bridge/link.c +1 -0

 		"                               [ self ] [ master ]\n"
 		"       bridge link show [dev DEV]\n");
 	exit(-1);

@@ -330,6 +335,7 @@ static int brlink_modify(int argc, char **argv)
 	};
 	char *d = NULL;
 	int backup_port_idx = -1;
+	__s32 backup_nhid = -1;
 	__s8 neigh_suppress = -1;
 	__s8 neigh_vlan_suppress = -1;
 	__s8 learning = -1;

Quoted 8 lines · Ido Schimmel

bridge/link.c +0 -0

index b35429866f52..c7ee5e760c08 100644
@@ -186,6 +186,10 @@ static void print_protinfo(FILE *fp, struct rtattr *attr)
 				     ll_index_to_name(ifidx));
 		}
 

bridge/link.c static void print_protinfo(FILE *fp, struct rtattr *attr) · line 192

Quoted 4 lines

bridge/link.c +4 -0

+		if (prtb[IFLA_BRPORT_BACKUP_NHID])
+			print_uint(PRINT_ANY, "backup_nhid", "backup_nhid %u ",
+				   rta_getattr_u32(prtb[IFLA_BRPORT_BACKUP_NHID]));
+

This doesn't build on current main. I think we usually send the relevant header sync patch, but maybe there's an assumption the maintainer pushes it _before_ this patch? I'm not sure, just calling it out.

bridge/link.c static void usage(void) · line 318

Quoted 9 lines

bridge/link.c +1 -0

 		if (prtb[IFLA_BRPORT_ISOLATED])
 			print_on_off(PRINT_ANY, "isolated", "isolated %s ",
 				     rta_getattr_u8(prtb[IFLA_BRPORT_ISOLATED]));

@@ -311,6 +315,7 @@ static void usage(void)
 		"                               [ mab {on | off} ]\n"
 		"                               [ hwmode {vepa | veb} ]\n"
 		"                               [ backup_port DEVICE ] [ nobackup_port ]\n"
+		"                               [ backup_nhid NHID ]\n"

I thought about whether there should be "nobackup_nhid", but no. The corresponding nobackup_port is necessary because it would be awkward to specify "backup_port ''" or something. No such issue with NHID.

Quoted 12 lines

bridge/link.c +1 -0

 		"                               [ self ] [ master ]\n"
 		"       bridge link show [dev DEV]\n");
 	exit(-1);

@@ -330,6 +335,7 @@ static int brlink_modify(int argc, char **argv)
 	};
 	char *d = NULL;
 	int backup_port_idx = -1;
+	__s32 backup_nhid = -1;
 	__s8 neigh_suppress = -1;
 	__s8 neigh_vlan_suppress = -1;
 	__s8 learning = -1;

bridge/link.c static int brlink_modify(int argc, char **argv) · line 505

Quoted 7 lines

bridge/link.c +4 -0

@@ -493,6 +499,10 @@ static int brlink_modify(int argc, char **argv)
 			}
 		} else if (strcmp(*argv, "nobackup_port") == 0) {
 			backup_port_idx = 0;
+		} else if (strcmp(*argv, "backup_nhid") == 0) {
+			NEXT_ARG();
+			if (get_s32(&backup_nhid, *argv, 0))
+				invarg("invalid backup_nhid", *argv);

Not sure about that s32. NHID's are unsigned in general. I can add a NHID of 0xffffffff just fine:

# ip nexthop add id 0xffffffff via 192.0.2.3 dev Xd

(Though ip nexthop show then loops endlessly probably because -1 is used as a sentinel in the dump code. Oops!)

IMHO the tool should allow configuring this. You allow full u32 range for the "ip" tool, no need for "bridge" to be arbitrarily limited.

Quoted 15 lines · Petr Machata

Review static int brlink_modify(int argc, char **argv) · line 505

Quoted 8 lines · Ido Schimmel

(quoted diff) +4 -0

@@ -493,6 +499,10 @@ static int brlink_modify(int argc, char **argv)
 			}
 		} else if (strcmp(*argv, "nobackup_port") == 0) {
 			backup_port_idx = 0;
+		} else if (strcmp(*argv, "backup_nhid") == 0) {
+			NEXT_ARG();
+			if (get_s32(&backup_nhid, *argv, 0))
+				invarg("invalid backup_nhid", *argv);

Not sure about that s32. NHID's are unsigned in general. I can add a NHID of 0xffffffff just fine:

Quoted 7 lines

# ip nexthop add id 0xffffffff via 192.0.2.3 dev Xd

(Though ip nexthop show then loops endlessly probably because -1 is used as a sentinel in the dump code. Oops!)

IMHO the tool should allow configuring this. You allow full u32 range for the "ip" tool, no need for "bridge" to be arbitrarily limited.

What about the diff below?

bridge/link.c +4 -2

index c7ee5e760c08..4bf806c5be61 100644
@@ -334,8 +334,9 @@ static int brlink_modify(int argc, char **argv)
                .ifm.ifi_family = PF_BRIDGE,
        };
        char *d = NULL;
+       bool backup_nhid_set = false;
+       __u32 backup_nhid;
        int backup_port_idx = -1;
-       __s32 backup_nhid = -1;
        __s8 neigh_suppress = -1;
        __s8 neigh_vlan_suppress = -1;
        __s8 learning = -1;

@@ -501,8 +502,9 @@ static int brlink_modify(int argc, char **argv)
                        backup_port_idx = 0;
                } else if (strcmp(*argv, "backup_nhid") == 0) {
                        NEXT_ARG();
-                       if (get_s32(&backup_nhid, *argv, 0))
+                       if (get_u32(&backup_nhid, *argv, 0))
                                invarg("invalid backup_nhid", *argv);
+                       backup_nhid_set = true;
                } else {
                        usage();
                }

Quoted 11 lines · Petr Machata

Quoted 8 lines · Ido Schimmel

bridge/link.c +0 -0

index b35429866f52..c7ee5e760c08 100644
@@ -186,6 +186,10 @@ static void print_protinfo(FILE *fp, struct rtattr *attr)
 				     ll_index_to_name(ifidx));
 		}
 

Quoted 8 lines

bridge/link.c static void print_protinfo(FILE *fp, struct rtattr *attr) · line 192

Quoted 4 lines

bridge/link.c +4 -0

+		if (prtb[IFLA_BRPORT_BACKUP_NHID])
+			print_uint(PRINT_ANY, "backup_nhid", "backup_nhid %u ",
+				   rta_getattr_u32(prtb[IFLA_BRPORT_BACKUP_NHID]));
+

This doesn't build on current main. I think we usually send the relevant header sync patch, but maybe there's an assumption the maintainer pushes it _before_ this patch? I'm not sure, just calling it out.

Not needed. David syncs the headers himself.

Quoted 38 lines

bridge/link.c static void usage(void) · line 318

Quoted 10 lines

bridge/link.c +1 -0

 		if (prtb[IFLA_BRPORT_ISOLATED])
 			print_on_off(PRINT_ANY, "isolated", "isolated %s ",
 				     rta_getattr_u8(prtb[IFLA_BRPORT_ISOLATED]));

@@ -311,6 +315,7 @@ static void usage(void)
 		"                               [ mab {on | off} ]\n"
 		"                               [ hwmode {vepa | veb} ]\n"
 		"                               [ backup_port DEVICE ] [ nobackup_port ]\n"
+		"                               [ backup_nhid NHID ]\n"

I thought about whether there should be "nobackup_nhid", but no. The corresponding nobackup_port is necessary because it would be awkward to specify "backup_port ''" or something. No such issue with NHID.

Quoted 12 lines

bridge/link.c +1 -0

 		"                               [ self ] [ master ]\n"
 		"       bridge link show [dev DEV]\n");
 	exit(-1);

@@ -330,6 +335,7 @@ static int brlink_modify(int argc, char **argv)
 	};
 	char *d = NULL;
 	int backup_port_idx = -1;
+	__s32 backup_nhid = -1;
 	__s8 neigh_suppress = -1;
 	__s8 neigh_vlan_suppress = -1;
 	__s8 learning = -1;

bridge/link.c static int brlink_modify(int argc, char **argv) · line 505

Quoted 7 lines

bridge/link.c +4 -0

@@ -493,6 +499,10 @@ static int brlink_modify(int argc, char **argv)
 			}
 		} else if (strcmp(*argv, "nobackup_port") == 0) {
 			backup_port_idx = 0;
+		} else if (strcmp(*argv, "backup_nhid") == 0) {
+			NEXT_ARG();
+			if (get_s32(&backup_nhid, *argv, 0))
+				invarg("invalid backup_nhid", *argv);

Not sure about that s32. NHID's are unsigned in general. I can add a NHID of 0xffffffff just fine:

Quoted 4 lines

# ip nexthop add id 0xffffffff via 192.0.2.3 dev Xd

(Though ip nexthop show then loops endlessly probably because -1 is used as a sentinel in the dump code. Oops!)

ugh, please send a fix.