[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID:
<MW3PR15MB3913AD3F5DCEB5D95C52DB85FA4EA@MW3PR15MB3913.namprd15.prod.outlook.com>
Date: Tue, 8 Jul 2025 23:12:21 +0000
From: David Wilder <wilder@...ibm.com>
To: Jay Vosburgh <jv@...sburgh.net>
CC: "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"pradeeps@...ux.vnet.ibm.com" <pradeeps@...ux.vnet.ibm.com>,
Pradeep
Satyanarayana <pradeep@...ibm.com>,
"i.maximets@....org"
<i.maximets@....org>,
Adrian Moreno Zapata <amorenoz@...hat.com>,
Hangbin Liu
<haliu@...hat.com>,
"stephen@...workplumber.org"
<stephen@...workplumber.org>,
"dsahern@...il.com" <dsahern@...il.com>
Subject: RE: [PATCH iproute2-next v1 1/1] iproute: Extend bonding's "arp_ip_target"
parameter to add vlan tags.
________________________________________
From: Jay Vosburgh <jv@...sburgh.net>
Sent: Tuesday, July 8, 2025 9:51 AM
To: David Wilder
Cc: netdev@...r.kernel.org; pradeeps@...ux.vnet.ibm.com; Pradeep Satyanarayana; i.maximets@....org; Adrian Moreno Zapata; Hangbin Liu; stephen@...workplumber.org; dsahern@...il.com
Subject: [EXTERNAL] Re: [PATCH iproute2-next v1 1/1] iproute: Extend bonding's "arp_ip_target" parameter to add vlan tags.
>>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@...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);
Answering your last question first, IFLA_BOND_ARP_IP_TARGET was
already NLA_NESTED (see: bond_netlink.c).
>
> 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).
I manage to preserve compatibility forward and backward. Both a new
kernel and old kernel will work with both an new and old iproute2.
> 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.
I thought adding a new API was what we wanted to avoid. But a new API
is unnecessary as I am extending the existing one in such a way to not
break the original api.
The original code sent 4 bytes of data (the 32bit ip address) in each nested
entry. The new code sends the same 4 bytes containing the ip address.
If a list of tags has been included the data is appended to the 4byte address.
Type NLA_NESTED has no fixed data size. If no vlans are supplied then the
data sent to the kernel looks exactly the same as with the old iproute2.
Therefor a new kernel will continue to work with the old iproute2 command,
you just cant add vlan tags until you upgrade iproute2.
An old kernel will continue to work with a new iproute2. The kernel will be
sent the same 4Byte address in each nested entry. If you try to add a list
of vlan tags the kernel ignores the extra data.
I tested the various combinations.
>
> 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@...sburgh.net
David Wilder wilder@...ibm.com
Powered by blists - more mailing lists