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

[David Ahern <dsahern@gmail.com>]

On 8/2/23 6:34 AM, Ido Schimmel wrote:
> On Wed, Aug 02, 2023 at 11:55:26AM +0200, Petr Machata wrote:
>>
>> 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.
> 
> Not needed. David syncs the headers himself.

just pushed the update.

> 
>>
>>>  		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!)

ugh, please send a fix.