[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <Za5eizfgzl5mwt50@shredder>
Date: Mon, 22 Jan 2024 14:24:43 +0200
From: Ido Schimmel <idosch@...sch.org>
To: Alce Lafranque <alce@...ranque.net>
Cc: netdev@...r.kernel.org, stephen@...workplumber.org,
Vincent Bernat <vincent@...nat.ch>, dsahern@...il.com
Subject: Re: [PATCH iproute2] vxlan: add support for flowlab inherit
s/flowlab/flowlabel/ in subject
My understanding is that new features should be targeted at
iproute2-next. See the README.
On Sat, Jan 20, 2024 at 06:44:18AM -0600, Alce Lafranque wrote:
> @@ -214,10 +214,16 @@ static int vxlan_parse_opt(struct link_util *lu, int argc, char **argv,
> NEXT_ARG();
> check_duparg(&attrs, IFLA_VXLAN_LABEL, "flowlabel",
> *argv);
> - if (get_u32(&uval, *argv, 0) ||
> - (uval & ~LABEL_MAX_MASK))
> - invarg("invalid flowlabel", *argv);
> - addattr32(n, 1024, IFLA_VXLAN_LABEL, htonl(uval));
> + if (strcmp(*argv, "inherit") == 0) {
> + addattr32(n, 1024, IFLA_VXLAN_LABEL_POLICY, VXLAN_LABEL_INHERIT);
> + } else {
> + if (get_u32(&uval, *argv, 0) ||
> + (uval & ~LABEL_MAX_MASK))
> + invarg("invalid flowlabel", *argv);
> + addattr32(n, 1024, IFLA_VXLAN_LABEL_POLICY, VXLAN_LABEL_FIXED);
I think I mentioned this during the review of the kernel patch, but the
current approach relies on old kernels ignoring the
'IFLA_VXLAN_LABEL_POLICY' attribute which is not nice. My personal
preference would be to add a new keyword for the new attribute:
# ip link set dev vx0 type vxlan flowlabel_policy inherit
# ip link set dev vx0 type vxlan flowlabel_policy fixed flowlabel 10
But let's see what David thinks.
> + addattr32(n, 1024, IFLA_VXLAN_LABEL,
> + htonl(uval));
> + }
> } else if (!matches(*argv, "ageing")) {
> __u32 age;
>
> @@ -580,12 +586,25 @@ static void vxlan_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[])
> print_string(PRINT_ANY, "df", "df %s ", "inherit");
> }
>
> - if (tb[IFLA_VXLAN_LABEL]) {
> - __u32 label = rta_getattr_u32(tb[IFLA_VXLAN_LABEL]);
> -
> - if (label)
> - print_0xhex(PRINT_ANY, "label",
> - "flowlabel %#llx ", ntohl(label));
> + enum ifla_vxlan_label_policy policy = VXLAN_LABEL_FIXED;
> + if (tb[IFLA_VXLAN_LABEL_POLICY]) {
> + policy = rta_getattr_u32(tb[IFLA_VXLAN_LABEL_POLICY]);
> + }
Checkpatch says:
WARNING: Missing a blank line after declarations
#112: FILE: ip/iplink_vxlan.c:590:
+ enum ifla_vxlan_label_policy policy = VXLAN_LABEL_FIXED;
+ if (tb[IFLA_VXLAN_LABEL_POLICY]) {
WARNING: braces {} are not necessary for single statement blocks
#112: FILE: ip/iplink_vxlan.c:590:
+ if (tb[IFLA_VXLAN_LABEL_POLICY]) {
+ policy = rta_getattr_u32(tb[IFLA_VXLAN_LABEL_POLICY]);
+ }
> + switch (policy) {
> + case VXLAN_LABEL_FIXED:
> + if (tb[IFLA_VXLAN_LABEL]) {
> + __u32 label = rta_getattr_u32(tb[IFLA_VXLAN_LABEL]);
> +
> + if (label)
> + print_0xhex(PRINT_ANY, "label",
> + "flowlabel %#llx ", ntohl(label));
> + }
> + break;
> + case VXLAN_LABEL_INHERIT:
> + print_string(PRINT_FP, NULL, "flowlabel %s ", "inherit");
> + break;
> + default:
> + break;
> }
>
> if (tb[IFLA_VXLAN_AGEING]) {
> --
> 2.39.2
>
>
Powered by blists - more mailing lists