Re: [PATCH iproute2] vxlan: add support for flowlab inherit

[Ido Schimmel <idosch@idosch.org>]

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