Re: [PATCH iproute2-next v1 1/1] iproute: Extend bonding's "arp_ip_target" parameter to add vlan tags.

[Jay Vosburgh <jv@jvosburgh.net>]

David Wilder <wilder@us.ibm.com> wrote:

>This change extends the "arp_ip_target" parameter format to allow for
>a list of vlan tags to be included for each arp target.
>
>The new format for arp_ip_target is:
>arp_ip_target=ipv4-address[vlan-tag\...],...
>
>Examples:
>arp_ip_target=10.0.0.1[10]
>arp_ip_target=10.0.0.1[100/200]
>
>Signed-off-by: David Wilder <wilder@us.ibm.com>
>---
> ip/iplink_bond.c | 62 +++++++++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 59 insertions(+), 3 deletions(-)
>
>diff --git a/ip/iplink_bond.c b/ip/iplink_bond.c
>index 19af67d0..bb0b6e84 100644
>--- a/ip/iplink_bond.c
>+++ b/ip/iplink_bond.c
>@@ -173,6 +173,45 @@ static void explain(void)
> 	print_explain(stderr);
> }
> 
>+#define BOND_VLAN_PROTO_NONE htons(0xffff)
>+
>+struct bond_vlan_tag {
>+	__be16	vlan_proto;
>+	__be16	vlan_id;
>+};
>+
>+static inline struct bond_vlan_tag *bond_vlan_tags_parse(char *vlan_list, int level, int *size)
>+{
>+	struct bond_vlan_tag *tags = NULL;
>+	char *vlan;
>+	int n;
>+
>+	if (!vlan_list || strlen(vlan_list) == 0) {
>+		tags = calloc(level + 1, sizeof(*tags));
>+		*size = (level + 1) * (sizeof(*tags));
>+		if (tags)
>+			tags[level].vlan_proto = BOND_VLAN_PROTO_NONE;
>+		return tags;
>+	}
>+
>+	for (vlan = strsep(&vlan_list, "/"); (vlan != 0); level++) {
>+		tags = bond_vlan_tags_parse(vlan_list, level + 1, size);
>+		if (!tags)
>+			continue;
>+
>+		tags[level].vlan_proto = htons(ETH_P_8021Q);
>+		n = sscanf(vlan, "%hu", &(tags[level].vlan_id));
>+
>+		if (n != 1 || tags[level].vlan_id < 1 ||
>+		    tags[level].vlan_id > 4094)
>+			return NULL;
>+
>+		return tags;
>+	}
>+
>+	return NULL;
>+}
>+
> static int bond_parse_opt(struct link_util *lu, int argc, char **argv,
> 			  struct nlmsghdr *n)
> {
>@@ -239,12 +278,29 @@ static int bond_parse_opt(struct link_util *lu, int argc, char **argv,
> 				NEXT_ARG();
> 				char *targets = strdupa(*argv);
> 				char *target = strtok(targets, ",");
>-				int i;
>+				struct bond_vlan_tag *tags;
>+				int size, i;
> 
> 				for (i = 0; target && i < BOND_MAX_ARP_TARGETS; i++) {
>-					__u32 addr = get_addr32(target);
>+					struct Data {
>+						__u32 addr;
>+						struct bond_vlan_tag vlans[];
>+					} data;
>+					char *vlan_list, *dup;
>+
>+					dup = strdupa(target);
>+					data.addr = get_addr32(strsep(&dup, "["));
>+					vlan_list = strsep(&dup, "]");
>+
>+					if (vlan_list) {
>+						tags = bond_vlan_tags_parse(vlan_list, 0, &size);
>+						memcpy(&data.vlans, tags, size);
>+						addattr_l(n, 1024, i, &data,
>+							  sizeof(data.addr)+size);
>+					} else {
>+						addattr32(n, 1024, i, data.addr);
>+					}
> 
>-					addattr32(n, 1024, i, addr);

	Another question occurred to me: is this method for sending the
VLAN tags going to break compatibility?  New versions of iproute2 need
to work on older kernels, so we can't simply change existing APIs in
ways that require a lockstep change of iproute versions (going either
forwards or backwards).

	The above looks like it changes the structure being conveyed
into the kernel, which I don't think we can do.  In the kernel, the old
API will need to continue to function, and therefore the new "with VLAN
tag" case will need to use a new API.

	The existing IFLA_BOND_ARP_IP_TARGET type doesn't use nested
netlink types, it just sends binary data, so I'm thinking we can't just
change that binary data, and would need a new IFLA_BOND_ type.

	-J

> 					target = strtok(NULL, ",");
> 				}
> 				addattr_nest_end(n, nest);
>-- 
>2.43.5
>

---
	-Jay Vosburgh, jv@jvosburgh.net