From: Ido Schimmel <idosch@idosch•org>
To: Petr Machata <petrm@nvidia•com>
Cc: Ido Schimmel <idosch@nvidia•com>,
netdev@vger•kernel.org, dsahern@gmail•com,
stephen@networkplumber•org, razor@blackwall•org
Subject: Re: [PATCH iproute2-next] bridge: Add backup nexthop ID support
Date: Wed, 2 Aug 2023 15:34:15 +0300 [thread overview]
Message-ID: <ZMpNRzXKIS7ZzSVN@shredder> (raw)
In-Reply-To: <87sf91enuf.fsf@nvidia.com>
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);
next prev parent reply other threads:[~2023-08-02 12:34 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
2023-08-02 13:35 ` Petr Machata
2023-08-02 15:35 ` David Ahern
2023-08-02 17:22 ` Ido Schimmel
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZMpNRzXKIS7ZzSVN@shredder \
--to=idosch@idosch$(echo .)org \
--cc=dsahern@gmail$(echo .)com \
--cc=idosch@nvidia$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
--cc=petrm@nvidia$(echo .)com \
--cc=razor@blackwall$(echo .)org \
--cc=stephen@networkplumber$(echo .)org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox