netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH iproute2-next] bridge: Add backup nexthop ID support
@ 2023-08-01 15:21 Ido Schimmel
  2023-08-02  9:55 ` Petr Machata
  0 siblings, 1 reply; 6+ messages in thread
From: Ido Schimmel @ 2023-08-01 15:21 UTC (permalink / raw)
  To: netdev; +Cc: dsahern, stephen, razor, petrm, Ido Schimmel

Extend the bridge and ip utilities to set and show the backup nexthop ID
bridge port attribute. A value of 0 (default) disables the feature, in
which case the attribute is not printed since it is not emitted by the
kernel.

Example:

 # bridge -d link show dev swp1 | grep -o "backup_nhid [0-9]*"
 # bridge -d -j -p link show dev swp1 | jq '.[]["backup_nhid"]'
 null

 # bridge link set dev swp1 backup_nhid 10
 # bridge -d link show dev swp1 | grep -o "backup_nhid [0-9]*"
 backup_nhid 10
 # bridge -d -j -p link show dev swp1 | jq '.[]["backup_nhid"]'
 10

 # bridge link set dev swp1 backup_nhid 0
 # bridge -d link show dev swp1 | grep -o "backup_nhid [0-9]*"
 # bridge -d -j -p link show dev swp1 | jq '.[]["backup_nhid"]'
 null

 # ip -d link show dev swp1 | grep -o "backup_nhid [0-9]*"
 # ip -d -j -p lin show dev swp1 | jq '.[]["linkinfo"]["info_slave_data"]["backup_nhid"]'
 null

 # ip link set dev swp1 type bridge_slave backup_nhid 10
 # ip -d link show dev swp1 | grep -o "backup_nhid [0-9]*"
 backup_nhid 10
 # ip -d -j -p lin show dev swp1 | jq '.[]["linkinfo"]["info_slave_data"]["backup_nhid"]'
 10

 # ip link set dev swp1 type bridge_slave backup_nhid 0
 # ip -d link show dev swp1 | grep -o "backup_nhid [0-9]*"
 # ip -d -j -p lin show dev swp1 | jq '.[]["linkinfo"]["info_slave_data"]["backup_nhid"]'
 null

Signed-off-by: Ido Schimmel <idosch@nvidia.com>
---
 bridge/link.c            | 14 ++++++++++++++
 ip/iplink_bridge_slave.c | 13 +++++++++++++
 man/man8/bridge.8        |  9 +++++++++
 man/man8/ip-link.8.in    | 11 ++++++++++-
 4 files changed, 46 insertions(+), 1 deletion(-)

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]));
+
 		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"
 		"                               [ 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);
 		} else {
 			usage();
 		}
@@ -579,6 +589,10 @@ static int brlink_modify(int argc, char **argv)
 		addattr32(&req.n, sizeof(req), IFLA_BRPORT_BACKUP_PORT,
 			  backup_port_idx);
 
+	if (backup_nhid != -1)
+		addattr32(&req.n, sizeof(req), IFLA_BRPORT_BACKUP_NHID,
+			  backup_nhid);
+
 	addattr_nest_end(&req.n, nest);
 
 	/* IFLA_AF_SPEC nested attribute. Contains IFLA_BRIDGE_FLAGS that
diff --git a/ip/iplink_bridge_slave.c b/ip/iplink_bridge_slave.c
index 11ab2113fe96..dc73c86574da 100644
--- a/ip/iplink_bridge_slave.c
+++ b/ip/iplink_bridge_slave.c
@@ -43,6 +43,7 @@ static void print_explain(FILE *f)
 		"			[ locked {on | off} ]\n"
 		"			[ mab {on | off} ]\n"
 		"			[ backup_port DEVICE ] [ nobackup_port ]\n"
+		"			[ backup_nhid NHID ]\n"
 	);
 }
 
@@ -301,6 +302,10 @@ static void bridge_slave_print_opt(struct link_util *lu, FILE *f,
 		print_string(PRINT_ANY, "backup_port", "backup_port %s ",
 			     ll_index_to_name(backup_p));
 	}
+
+	if (tb[IFLA_BRPORT_BACKUP_NHID])
+		print_uint(PRINT_ANY, "backup_nhid", "backup_nhid %u ",
+			   rta_getattr_u32(tb[IFLA_BRPORT_BACKUP_NHID]));
 }
 
 static void bridge_slave_parse_on_off(char *arg_name, char *arg_val,
@@ -436,6 +441,14 @@ static int bridge_slave_parse_opt(struct link_util *lu, int argc, char **argv,
 			addattr32(n, 1024, IFLA_BRPORT_BACKUP_PORT, ifindex);
 		} else if (matches(*argv, "nobackup_port") == 0) {
 			addattr32(n, 1024, IFLA_BRPORT_BACKUP_PORT, 0);
+		} else if (strcmp(*argv, "backup_nhid") == 0) {
+			__u32 backup_nhid;
+
+			NEXT_ARG();
+			if (get_u32(&backup_nhid, *argv, 0))
+				invarg("backup_nhid is invalid", *argv);
+			addattr32(n, 1024, IFLA_BRPORT_BACKUP_NHID,
+				  backup_nhid);
 		} else if (matches(*argv, "help") == 0) {
 			explain();
 			return -1;
diff --git a/man/man8/bridge.8 b/man/man8/bridge.8
index e05528199eab..dd0659d37df2 100644
--- a/man/man8/bridge.8
+++ b/man/man8/bridge.8
@@ -61,6 +61,8 @@ bridge \- show / manipulate bridge addresses and devices
 .B backup_port
 .IR  DEVICE " ] ["
 .BR nobackup_port " ] [ "
+.B backup_nhid
+.IR NHID " ] ["
 .BR self " ] [ " master " ]"
 
 .ti -8
@@ -647,6 +649,13 @@ configured backup port
 .B nobackup_port
 Removes the currently configured backup port
 
+.TP
+.BI backup_nhid " NHID"
+The FDB nexthop object ID (see \fBip-nexthop\fR(8)) to attach to packets being
+redirected to a backup port that has VLAN tunnel mapping enabled (via the
+\fBvlan_tunnel\fR option). Setting a value of 0 (default) has the effect of not
+attaching any ID.
+
 .TP
 .B self
 link setting is configured on specified physical device
diff --git a/man/man8/ip-link.8.in b/man/man8/ip-link.8.in
index 6a82ddc45adf..128c855f82be 100644
--- a/man/man8/ip-link.8.in
+++ b/man/man8/ip-link.8.in
@@ -2539,7 +2539,10 @@ the following additional arguments are supported:
 ] [
 .BR backup_port " DEVICE"
 ] [
-.BR nobackup_port " ]"
+.BR nobackup_port
+] [
+.BR backup_nhid " NHID"
+]
 
 .in +8
 .sp
@@ -2678,6 +2681,12 @@ configured backup port
 .BR nobackup_port
 - removes the currently configured backup port
 
+.BI backup_nhid " NHID"
+- the FDB nexthop object ID (see \fBip-nexthop\fR(8)) to attach to packets
+being redirected to a backup port that has VLAN tunnel mapping enabled (via the
+\fBvlan_tunnel\fR option). Setting a value of 0 (default) has the effect of not
+attaching any ID.
+
 .in -8
 
 .TP
-- 
2.40.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH iproute2-next] bridge: Add backup nexthop ID support
  2023-08-01 15:21 [PATCH iproute2-next] bridge: Add backup nexthop ID support Ido Schimmel
@ 2023-08-02  9:55 ` Petr Machata
  2023-08-02 12:34   ` Ido Schimmel
  0 siblings, 1 reply; 6+ messages in thread
From: Petr Machata @ 2023-08-02  9:55 UTC (permalink / raw)
  To: Ido Schimmel; +Cc: netdev, dsahern, stephen, razor, petrm


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.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH iproute2-next] bridge: Add backup nexthop ID support
  2023-08-02  9:55 ` Petr Machata
@ 2023-08-02 12:34   ` Ido Schimmel
  2023-08-02 13:35     ` Petr Machata
  2023-08-02 15:35     ` David Ahern
  0 siblings, 2 replies; 6+ messages in thread
From: Ido Schimmel @ 2023-08-02 12:34 UTC (permalink / raw)
  To: Petr Machata; +Cc: Ido Schimmel, netdev, dsahern, stephen, razor

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.

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

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

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH iproute2-next] bridge: Add backup nexthop ID support
  2023-08-02 12:34   ` Ido Schimmel
@ 2023-08-02 13:35     ` Petr Machata
  2023-08-02 15:35     ` David Ahern
  1 sibling, 0 replies; 6+ messages in thread
From: Petr Machata @ 2023-08-02 13:35 UTC (permalink / raw)
  To: Ido Schimmel; +Cc: Petr Machata, Ido Schimmel, netdev, dsahern, stephen, razor


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>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH iproute2-next] bridge: Add backup nexthop ID support
  2023-08-02 12:34   ` Ido Schimmel
  2023-08-02 13:35     ` Petr Machata
@ 2023-08-02 15:35     ` David Ahern
  2023-08-02 17:22       ` Ido Schimmel
  1 sibling, 1 reply; 6+ messages in thread
From: David Ahern @ 2023-08-02 15:35 UTC (permalink / raw)
  To: Ido Schimmel, Petr Machata; +Cc: Ido Schimmel, netdev, stephen, razor

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.


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH iproute2-next] bridge: Add backup nexthop ID support
  2023-08-02 15:35     ` David Ahern
@ 2023-08-02 17:22       ` Ido Schimmel
  0 siblings, 0 replies; 6+ messages in thread
From: Ido Schimmel @ 2023-08-02 17:22 UTC (permalink / raw)
  To: David Ahern; +Cc: Petr Machata, Ido Schimmel, netdev, stephen, razor

On Wed, Aug 02, 2023 at 09:35:12AM -0600, David Ahern wrote:
> ugh, please send a fix.

Will send a fix tomorrow. Seems to be fixed by [1], but we might need
something similar for the bucket dump.

[1]
diff --git a/net/ipv4/nexthop.c b/net/ipv4/nexthop.c
index 93f14d39fef6..1bd92acbc6c5 100644
--- a/net/ipv4/nexthop.c
+++ b/net/ipv4/nexthop.c
@@ -3170,7 +3170,7 @@ static int nh_valid_dump_req(const struct nlmsghdr *nlh,
 }
 
 struct rtm_dump_nh_ctx {
-       u32 idx;
+       u64 idx;
 };
 
 static struct rtm_dump_nh_ctx *
@@ -3192,7 +3192,7 @@ static int rtm_dump_walk_nexthops(struct sk_buff *skb,
                                  void *data)
 {
        struct rb_node *node;
-       int s_idx;
+       u64 s_idx;
        int err;
 
        s_idx = ctx->idx;

^ permalink raw reply related	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2023-08-02 17:22 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-08-01 15:21 [PATCH iproute2-next] bridge: Add backup nexthop ID support Ido Schimmel
2023-08-02  9:55 ` Petr Machata
2023-08-02 12:34   ` Ido Schimmel
2023-08-02 13:35     ` Petr Machata
2023-08-02 15:35     ` David Ahern
2023-08-02 17:22       ` Ido Schimmel

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).