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

[Petr Machata <petrm@nvidia.com>]

Ido Schimmel <idosch@idosch.org> writes:

> On Wed, Aug 02, 2023 at 11:55:26AM +0200, Petr Machata wrote:
>> 
>> Ido Schimmel <idosch@nvidia.com> writes:
>> 
>> > @@ -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.
>
> What about the diff below?
>
> diff --git a/bridge/link.c b/bridge/link.c
> index c7ee5e760c08..4bf806c5be61 100644
> --- a/bridge/link.c
> +++ b/bridge/link.c
> @@ -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();
>                 }
> @@ -589,7 +591,7 @@ static int brlink_modify(int argc, char **argv)
>                 addattr32(&req.n, sizeof(req), IFLA_BRPORT_BACKUP_PORT,
>                           backup_port_idx);
>  
> -       if (backup_nhid != -1)
> +       if (backup_nhid_set)
>                 addattr32(&req.n, sizeof(req), IFLA_BRPORT_BACKUP_NHID,
>                           backup_nhid);

Yep, that's what I had in mind.

With that:
Reviewed-by: Petr Machata <petrm@nvidia.com>