From: Petr Machata <petrm@nvidia•com>
To: Ido Schimmel <idosch@idosch•org>
Cc: Petr Machata <petrm@nvidia•com>, 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:35:20 +0200 [thread overview]
Message-ID: <873511efbg.fsf@nvidia.com> (raw)
In-Reply-To: <ZMpNRzXKIS7ZzSVN@shredder>
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>
next prev parent reply other threads:[~2023-08-02 13:47 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
2023-08-02 13:35 ` Petr Machata [this message]
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=873511efbg.fsf@nvidia.com \
--to=petrm@nvidia$(echo .)com \
--cc=dsahern@gmail$(echo .)com \
--cc=idosch@idosch$(echo .)org \
--cc=idosch@nvidia$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
--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