Re: [PATCH iproute2-next] bridge: Add backup nexthop ID support

[Petr Machata <petrm@nvidia.com>]

Ido Schimmel <idosch@nvidia.com> writes:

> diff --git a/bridge/link.c b/bridge/link.c
> index b35429866f52..c7ee5e760c08 100644
> --- a/bridge/link.c
> +++ b/bridge/link.c
> @@ -186,6 +186,10 @@ static void print_protinfo(FILE *fp, struct rtattr *attr)
>  				     ll_index_to_name(ifidx));
>  		}
>  
> +		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.

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

>  		"                               [ 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;
> @@ -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.