[PATCH iproute2] vxlan: allow specifying multiple default destinations

[PATCH iproute2] vxlan: allow specifying multiple default destinations

[Mike Rapoport]

Signed-off-by: Mike Rapoport <mike.rapoport@ravellosystems.com>
---
This patch depends on the pending changes to ip/iplink_vxlan.c as as
well as on IPv6 support in vxlan. I'll rebase and resend it once all
the changes to vxlan are merged.

 ip/iplink_vxlan.c | 84 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 84 insertions(+)

diff --git a/ip/iplink_vxlan.c b/ip/iplink_vxlan.c
index 661ab9e..9ba5a1d 100644
--- a/ip/iplink_vxlan.c
+++ b/ip/iplink_vxlan.c
@@ -28,11 +28,56 @@ static void explain(void)
 	fprintf(stderr, "                 [ port MIN MAX ] [ [no]learning ]\n");
 	fprintf(stderr, "                 [ [no]proxy ] [ [no]rsc ]\n");
 	fprintf(stderr, "                 [ [no]l2miss ] [ [no]l3miss ]\n");
+	fprintf(stderr, "                 [ dstadd DST ]\n");
+	fprintf(stderr, "                 [ dstdel ADDR ]\n");
 	fprintf(stderr, "\n");
 	fprintf(stderr, "Where: VNI := 0-16777215\n");
 	fprintf(stderr, "       ADDR := { IP_ADDRESS | any }\n");
 	fprintf(stderr, "       TOS  := { NUMBER | inherit }\n");
 	fprintf(stderr, "       TTL  := { 1..255 | inherit }\n");
+	fprintf(stderr, "       DST  := [ ADDR [port PORT] [vni VNI] [via DEV]]\n");
+}
+
+static int vxlan_parse_dst(int *argcp, char ***argvp, struct nlmsghdr *n)
+{
+	int argc = *argcp;
+	char **argv = *argvp;
+	__u32 vni, port, ifindex;
+	struct rtattr *tail;
+
+	tail = NLMSG_TAIL(n);
+	addattr_l(n, 1024, IFLA_VXLAN_REMOTE_ADD, NULL, 0);
+
+	while (argc > 0) {
+		if (!matches(*argv, "vni")) {
+			NEXT_ARG();
+			if (get_u32(&vni, *argv, 0) ||
+			    vni >= 1u << 24)
+				invarg("invalid id", *argv);
+			addattr32(n, 1024, IFLA_VXLAN_REMOTE_VNI, vni);
+		} else if (!matches(*argv, "port")) {
+			NEXT_ARG();
+			if (get_u32(&port, *argv, 0))
+				invarg("port", *argv);
+			addattr32(n, 1024, IFLA_VXLAN_REMOTE_PORT, port);
+		} else if (!matches(*argv, "via")) {
+			NEXT_ARG();
+			ifindex = if_nametoindex(*argv);
+			addattr32(n, 1024, IFLA_VXLAN_REMOTE_IFINDEX, ifindex);
+		} else {
+			inet_prefix addr;
+			get_prefix(&addr, *argv, AF_UNSPEC);
+			addattr_l(n, 1024, IFLA_VXLAN_REMOTE_ADDR,
+				  &addr.data, addr.bytelen);
+		}
+		argc--, argv++;
+	}
+
+	tail->rta_len = (void *) NLMSG_TAIL(n) - (void *)tail;
+
+	*argcp = argc;
+	*argvp = argv;
+	return 0;
 }
 
 static int vxlan_parse_opt(struct link_util *lu, int argc, char **argv,
@@ -56,6 +101,7 @@ static int vxlan_parse_opt(struct link_util *lu, int argc, char **argv,
 	__u32 age = 0;
 	__u32 maxaddr = 0;
 	struct ifla_vxlan_port_range range = { 0, 0 };
+	inet_prefix *remote_del = NULL;
 
 	while (argc > 0) {
 		if (!matches(*argv, "id") ||
@@ -138,6 +184,14 @@ static int vxlan_parse_opt(struct link_util *lu, int argc, char **argv,
 				invarg("max port", *argv);
 			range.low = htons(minport);
 			range.high = htons(maxport);
+		} else if (!matches(*argv, "dstadd")) {
+			NEXT_ARG();
+			vxlan_parse_dst(&argc, &argv, n);
+		} else if (!matches(*argv, "dstdel")) {
+			inet_prefix addr;
+			NEXT_ARG();
+			get_prefix(&addr, *argv, AF_UNSPEC);
+			remote_del = &addr;
 		} else if (!matches(*argv, "nolearning")) {
 			learning = 0;
 		} else if (!matches(*argv, "learning")) {
@@ -202,10 +256,37 @@ static int vxlan_parse_opt(struct link_util *lu, int argc, char **argv,
 	if (range.low || range.high)
 		addattr_l(n, 1024, IFLA_VXLAN_PORT_RANGE,
 			  &range, sizeof(range));
+	if (remote_del)
+		addattr_l(n, 1024, IFLA_VXLAN_REMOTE_DEL, remote_del->data,
+			  remote_del->bytelen);
 
 	return 0;
 }
 
+static void vxlan_print_remotes(FILE *f, struct rtattr *attr)
+{
+	struct rtattr *i;
+	char s1[1024];
+	int rem, n = 0;
+
+	fprintf(f, "\n      default destinations :\n");
+
+	rem = RTA_PAYLOAD(attr);
+	for (i = RTA_DATA(attr); RTA_OK(i, rem); i = RTA_NEXT(i, rem), n++) {
+		if (RTA_PAYLOAD(i) >= sizeof(struct in6_addr)) {
+			struct in6_addr addr;
+			memcpy(&addr, RTA_DATA(i), sizeof(struct in6_addr));
+			fprintf(f, "        %s\n",
+				format_host(AF_INET6, sizeof(struct in6_addr),
+					    &addr, s1, sizeof(s1)));
+		} else if (RTA_PAYLOAD(i) >= sizeof(__be32)) {
+			__be32 addr = rta_getattr_u32(i);
+			fprintf(f, "        %s\n",
+				format_host(AF_INET, 4, &addr, s1, sizeof(s1)));
+		}
+	}
+}
+
 static void vxlan_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[])
 {
 	__u32 vni;
@@ -308,6 +389,9 @@ static void vxlan_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[])
 	if (tb[IFLA_VXLAN_LIMIT] &&
 	    (maxaddr = rta_getattr_u32(tb[IFLA_VXLAN_LIMIT]) != 0))
 		    fprintf(f, "maxaddr %u ", maxaddr);
+
+	if (tb[IFLA_VXLAN_REMOTE_LST])
+		vxlan_print_remotes(f, tb[IFLA_VXLAN_REMOTE_LST]);
 }
 
 struct link_util vxlan_link_util = {
-- 
1.8.1.5

[PATCH net-next v2 0/2] vxlan: allow specifying multiple default destinations

[Mike Rapoport]

Hi,

These patches add ability to specify multiple default destinations to
vxlan. This ability is usefull in cases when multicast are disabled on
infrastructure level, for instance in public clouds.

v2 changes:
* rebased on current net-next
* flush default destinations list at dellink as per Atzm Watanabe comment
* support only IPv4

Mike Rapoport (2):
  vxlan: introduce vxlan_rdst_append
  vxlan: allow specifying multiple default destinations

 drivers/net/vxlan.c          | 164 +++++++++++++++++++++++++++++++++++++++++--
 include/uapi/linux/if_link.h |  14 ++++
 2 files changed, 174 insertions(+), 4 deletions(-)

-- 
1.8.1.5

[PATCH net-next v2 1/2] vxlan: introduce vxlan_rdst_append

[Mike Rapoport]

to allow remotes list management for both FDB entries and default
destinations

Signed-off-by: Mike Rapoport <mike.rapoport@ravellosystems.com>
---
 drivers/net/vxlan.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index 5ed64d4..e7facb8 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -367,14 +367,13 @@ static struct vxlan_fdb *vxlan_find_mac(struct vxlan_dev *vxlan,
 	return f;
 }
 
-/* Add/update destinations for multicast */
-static int vxlan_fdb_append(struct vxlan_fdb *f,
-			    __be32 ip, __be16 port, __u32 vni, __u32 ifindex)
+static int vxlan_rdst_append(struct vxlan_rdst *rdst, __be32 ip, __be16 port,
+			     __u32 vni, __u32 ifindex)
 {
 	struct vxlan_rdst *rd_prev, *rd;
 
 	rd_prev = NULL;
-	for (rd = &f->remote; rd; rd = rd->remote_next) {
+	for (rd = rdst; rd; rd = rd->remote_next) {
 		if (rd->remote_ip == ip &&
 		    rd->remote_port == port &&
 		    rd->remote_vni == vni &&
@@ -394,6 +393,13 @@ static int vxlan_fdb_append(struct vxlan_fdb *f,
 	return 1;
 }
 
+/* Add/update destinations for multicast */
+static int vxlan_fdb_append(struct vxlan_fdb *f,
+			    __be32 ip, __be16 port, __u32 vni, __u32 ifindex)
+{
+	return vxlan_rdst_append(&f->remote, ip, port, vni, ifindex);
+}
+
 /* Add new entry to forwarding table -- assumes lock held */
 static int vxlan_fdb_create(struct vxlan_dev *vxlan,
 			    const u8 *mac, __be32 ip,
-- 
1.8.1.5

[PATCH net-next v2 2/2] vxlan: allow specifying multiple default destinations

[Mike Rapoport]

A list of multiple default destinations can be used in environments that
disable multicast on the infrastructure level, e.g. public clouds.

Signed-off-by: Mike Rapoport <mike.rapoport@ravellosystems.com>
---
 drivers/net/vxlan.c          | 150 +++++++++++++++++++++++++++++++++++++++++++
 include/uapi/linux/if_link.h |  14 ++++
 2 files changed, 164 insertions(+)

diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index e7facb8..f33ea9d 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -138,6 +138,8 @@ struct vxlan_dev {
 	unsigned int	  addrcnt;
 	unsigned int	  addrmax;
 
+	unsigned int	  remote_cnt;  /* for additional default destinations */
+
 	struct hlist_head fdb_head[FDB_HASH_SIZE];
 };
 
@@ -643,6 +645,92 @@ static void vxlan_snoop(struct net_device *dev,
 	}
 }
 
+/* Add remote to default destinations list */
+static int vxlan_remote_add(struct vxlan_dev *vxlan, struct nlattr *attr)
+{
+	struct nlattr *i;
+	__be32 ip;
+	__be16 port;
+	u32 ifindex, vni;
+	int rem, err = 0;
+	bool addr_set = false;
+
+	port = vxlan->dst_port;
+	vni = vxlan->default_dst.remote_vni;
+	ifindex = vxlan->default_dst.remote_ifindex;
+
+	nla_for_each_nested(i, attr, rem) {
+		switch (nla_type(i)) {
+		case IFLA_VXLAN_REMOTE_ADDR:
+			ip = nla_get_be32(i);
+			addr_set = true;
+			break;
+		case IFLA_VXLAN_REMOTE_PORT:
+			port = nla_get_be16(i);
+			break;
+		case IFLA_VXLAN_REMOTE_VNI:
+			vni = nla_get_u32(i);
+			break;
+		case IFLA_VXLAN_REMOTE_IFINDEX:
+			ifindex = nla_get_u32(i);
+			break;
+		default:
+			err = -EINVAL;
+			break;
+		};
+
+		if (err)
+			return err;
+	}
+
+	if (!addr_set)
+		return -EINVAL;
+
+	err = vxlan_rdst_append(&vxlan->default_dst, ip, port, vni, ifindex);
+	if (err < 0)
+		return err;
+
+	if (err == 0)
+		return -EEXIST;
+
+	vxlan->remote_cnt++;
+
+	netdev_dbg(vxlan->dev, "dstadd %pI4\n", &ip);
+
+	return 0;
+}
+
+static void vxlan_remote_destroy(struct vxlan_dev *vxlan,
+				 struct vxlan_rdst *rd)
+{
+	__be32 ip = rd->remote_ip;
+	netdev_dbg(vxlan->dev, "dstdel %pI4\n", &ip);
+
+	--vxlan->remote_cnt;
+	kfree(rd);
+}
+
+/* Delete remote from default destinations list */
+static int vxlan_remote_delete(struct vxlan_dev *vxlan, struct nlattr *attr)
+{
+	struct vxlan_rdst *rd, *rd_prev = NULL;
+	__be32 ip;
+
+	ip = nla_get_be32(attr);
+
+	rd_prev = &vxlan->default_dst;
+
+	for (rd = vxlan->default_dst.remote_next; rd; rd = rd->remote_next) {
+		if (rd->remote_ip == ip) {
+			rd_prev->remote_next = rd->remote_next;
+			vxlan_remote_destroy(vxlan, rd);
+			return 0;
+		}
+		rd_prev = rd;
+	}
+
+	return -ENOENT;
+}
 
 /* See if multicast group is already in use by other ID */
 static bool vxlan_group_used(struct vxlan_net *vn,
@@ -1513,6 +1601,27 @@ static struct vxlan_sock *vxlan_socket_create(struct net *net, __be16 port)
 	return vs;
 }
 
+static int vxlan_changelink(struct net_device *dev,
+			    struct nlattr *tb[], struct nlattr *data[])
+{
+	struct vxlan_dev *vxlan = netdev_priv(dev);
+	int err;
+
+	if (data[IFLA_VXLAN_REMOTE_ADD]) {
+		err = vxlan_remote_add(vxlan, data[IFLA_VXLAN_REMOTE_ADD]);
+		if (err)
+			return err;
+	}
+
+	if (data[IFLA_VXLAN_REMOTE_DEL]) {
+		err = vxlan_remote_delete(vxlan, data[IFLA_VXLAN_REMOTE_DEL]);
+		if (err)
+			return err;
+	}
+
+	return 0;
+}
+
 static int vxlan_newlink(struct net *net, struct net_device *dev,
 			 struct nlattr *tb[], struct nlattr *data[])
 {
@@ -1631,11 +1740,20 @@ static int vxlan_newlink(struct net *net, struct net_device *dev,
 	return 0;
 }
 
+static void vxlan_remotes_flush(struct vxlan_dev *vxlan)
+{
+	struct vxlan_rdst *rd;
+
+	for (rd = vxlan->default_dst.remote_next; rd; rd = rd->remote_next)
+		vxlan_remote_destroy(vxlan, rd);
+}
+
 static void vxlan_dellink(struct net_device *dev, struct list_head *head)
 {
 	struct vxlan_dev *vxlan = netdev_priv(dev);
 	struct vxlan_sock *vs = vxlan->vn_sock;
 
+	vxlan_remotes_flush(vxlan);
 	hlist_del_rcu(&vxlan->hlist);
 	list_del(&vxlan->next);
 	unregister_netdevice_queue(dev, head);
@@ -1646,6 +1764,18 @@ static void vxlan_dellink(struct net_device *dev, struct list_head *head)
 	}
 }
 
+static size_t vxlan_remote_list_size(const struct net_device *dev)
+{
+	struct vxlan_dev *vxlan = netdev_priv(dev);
+	ssize_t size = nla_total_size(sizeof(struct nlattr));
+	struct vxlan_rdst *rd;
+
+	for (rd = vxlan->default_dst.remote_next; rd; rd = rd->remote_next)
+		size += nla_total_size(sizeof(__be32));
+
+	return size;
+}
+
 static size_t vxlan_get_size(const struct net_device *dev)
 {
 
@@ -1664,6 +1794,7 @@ static size_t vxlan_get_size(const struct net_device *dev)
 		nla_total_size(sizeof(__u32)) +	/* IFLA_VXLAN_LIMIT */
 		nla_total_size(sizeof(struct ifla_vxlan_port_range)) +
 		nla_total_size(sizeof(__be16))+ /* IFLA_VXLAN_PORT */
+		vxlan_remote_list_size(dev) +
 		0;
 }
 
@@ -1707,6 +1838,24 @@ static int vxlan_fill_info(struct sk_buff *skb, const struct net_device *dev)
 	if (nla_put(skb, IFLA_VXLAN_PORT_RANGE, sizeof(ports), &ports))
 		goto nla_put_failure;
 
+	if (vxlan->remote_cnt) {
+		struct vxlan_rdst *rdst;
+		struct nlattr *nest;
+
+		nest = nla_nest_start(skb, IFLA_VXLAN_REMOTE_LST);
+		if (nest == NULL)
+			goto nla_put_failure;
+
+		for (rdst = vxlan->default_dst.remote_next; rdst;
+		     rdst = rdst->remote_next) {
+			__be32 ip = rdst->remote_ip;
+			if (nla_put_be32(skb, IFLA_VXLAN_REMOTE_ADDR, ip))
+				goto nla_put_failure;
+		}
+
+		nla_nest_end(skb, nest);
+	}
+
 	return 0;
 
 nla_put_failure:
@@ -1721,6 +1870,7 @@ static struct rtnl_link_ops vxlan_link_ops __read_mostly = {
 	.setup		= vxlan_setup,
 	.validate	= vxlan_validate,
 	.newlink	= vxlan_newlink,
+	.changelink	= vxlan_changelink,
 	.dellink	= vxlan_dellink,
 	.get_size	= vxlan_get_size,
 	.fill_info	= vxlan_fill_info,
diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
index b05823c..5cab372 100644
--- a/include/uapi/linux/if_link.h
+++ b/include/uapi/linux/if_link.h
@@ -311,10 +311,24 @@ enum {
 	IFLA_VXLAN_L2MISS,
 	IFLA_VXLAN_L3MISS,
 	IFLA_VXLAN_PORT,	/* destination port */
+	IFLA_VXLAN_REMOTE_ADD,
+	IFLA_VXLAN_REMOTE_DEL,
+	IFLA_VXLAN_REMOTE_LST,
 	__IFLA_VXLAN_MAX
 };
 #define IFLA_VXLAN_MAX	(__IFLA_VXLAN_MAX - 1)
 
+enum {
+	IFLA_VXLAN_REMOTE_UNSPEC,
+	IFLA_VXLAN_REMOTE_ADDR,
+	IFLA_VXLAN_REMOTE_IFINDEX,
+	IFLA_VXLAN_REMOTE_PORT,
+	IFLA_VXLAN_REMOTE_VNI,
+	__IFLA_VXLAN_REMOTE_MAX
+};
+
+#define IFLA_VXLAN_REMOTE_MAX	(__IFLA_VXLAN_REMOTE_MAX - 1)
+
 struct ifla_vxlan_port_range {
 	__be16	low;
 	__be16	high;
-- 
1.8.1.5

[PATCH iproute2] vxlan: allow specifying multiple default destinations

[Mike Rapoport]

Signed-off-by: Mike Rapoport <mike.rapoport@ravellosystems.com>
---
 ip/iplink_vxlan.c | 85 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 85 insertions(+)

diff --git a/ip/iplink_vxlan.c b/ip/iplink_vxlan.c
index 1025326..89ca3c2 100644
--- a/ip/iplink_vxlan.c
+++ b/ip/iplink_vxlan.c
@@ -28,11 +28,57 @@ static void explain(void)
 	fprintf(stderr, "                 [ port MIN MAX ] [ [no]learning ]\n");
 	fprintf(stderr, "                 [ [no]proxy ] [ [no]rsc ]\n");
 	fprintf(stderr, "                 [ [no]l2miss ] [ [no]l3miss ]\n");
+	fprintf(stderr, "                 [ dstadd DST ]\n");
+	fprintf(stderr, "                 [ dstdel ADDR ]\n");
 	fprintf(stderr, "\n");
 	fprintf(stderr, "Where: VNI := 0-16777215\n");
 	fprintf(stderr, "       ADDR := { IP_ADDRESS | any }\n");
 	fprintf(stderr, "       TOS  := { NUMBER | inherit }\n");
 	fprintf(stderr, "       TTL  := { 1..255 | inherit }\n");
+	fprintf(stderr, "       DST  := [ ADDR [port PORT] [vni VNI] [via DEV]]\n");
+}
+
+static int vxlan_parse_dst(int *argcp, char ***argvp, struct nlmsghdr *n)
+{
+	int argc = *argcp;
+	char **argv = *argvp;
+	__u32 vni, ifindex;
+	__u16 port;
+	struct rtattr *tail;
+
+	tail = NLMSG_TAIL(n);
+	addattr_l(n, 1024, IFLA_VXLAN_REMOTE_ADD, NULL, 0);
+
+	while (argc > 0) {
+		if (!matches(*argv, "vni")) {
+			NEXT_ARG();
+			if (get_u32(&vni, *argv, 0) ||
+			    vni >= 1u << 24)
+				invarg("invalid id", *argv);
+			addattr32(n, 1024, IFLA_VXLAN_REMOTE_VNI, vni);
+		} else if (!matches(*argv, "port")) {
+			NEXT_ARG();
+			if (get_u16(&port, *argv, 0))
+				invarg("port", *argv);
+			addattr32(n, 1024, IFLA_VXLAN_REMOTE_PORT, htons(port));
+		} else if (!matches(*argv, "via")) {
+			NEXT_ARG();
+			ifindex = if_nametoindex(*argv);
+			addattr32(n, 1024, IFLA_VXLAN_REMOTE_IFINDEX, ifindex);
+		} else {
+			inet_prefix addr;
+			get_prefix(&addr, *argv, AF_UNSPEC);
+			addattr_l(n, 1024, IFLA_VXLAN_REMOTE_ADDR,
+				  &addr.data, addr.bytelen);
+		}
+		argc--, argv++;
+	}
+
+	tail->rta_len = (void *) NLMSG_TAIL(n) - (void *)tail;
+
+	*argcp = argc;
+	*argvp = argv;
+	return 0;
 }
 
 static int vxlan_parse_opt(struct link_util *lu, int argc, char **argv,
@@ -54,6 +100,7 @@ static int vxlan_parse_opt(struct link_util *lu, int argc, char **argv,
 	__u32 age = 0;
 	__u32 maxaddr = 0;
 	struct ifla_vxlan_port_range range = { 0, 0 };
+	inet_prefix *remote_del = NULL;
 
 	while (argc > 0) {
 		if (!matches(*argv, "id") ||
@@ -125,6 +172,14 @@ static int vxlan_parse_opt(struct link_util *lu, int argc, char **argv,
 				invarg("max port", *argv);
 			range.low = htons(minport);
 			range.high = htons(maxport);
+		} else if (!matches(*argv, "dstadd")) {
+			NEXT_ARG();
+			vxlan_parse_dst(&argc, &argv, n);
+		} else if (!matches(*argv, "dstdel")) {
+			inet_prefix addr;
+			NEXT_ARG();
+			get_prefix(&addr, *argv, AF_UNSPEC);
+			remote_del = &addr;
 		} else if (!matches(*argv, "nolearning")) {
 			learning = 0;
 		} else if (!matches(*argv, "learning")) {
@@ -183,10 +238,37 @@ static int vxlan_parse_opt(struct link_util *lu, int argc, char **argv,
 	if (range.low || range.high)
 		addattr_l(n, 1024, IFLA_VXLAN_PORT_RANGE,
 			  &range, sizeof(range));
+	if (remote_del)
+		addattr_l(n, 1024, IFLA_VXLAN_REMOTE_DEL, remote_del->data,
+			  remote_del->bytelen);
 
 	return 0;
 }
 
+static void vxlan_print_remotes(FILE *f, struct rtattr *attr)
+{
+	struct rtattr *i;
+	char s1[1024];
+	int rem, n = 0;
+
+	fprintf(f, "\n      default destinations :\n");
+
+	rem = RTA_PAYLOAD(attr);
+	for (i = RTA_DATA(attr); RTA_OK(i, rem); i = RTA_NEXT(i, rem), n++) {
+		if (RTA_PAYLOAD(i) >= sizeof(struct in6_addr)) {
+			struct in6_addr addr;
+			memcpy(&addr, RTA_DATA(i), sizeof(struct in6_addr));
+			fprintf(f, "        %s\n",
+				format_host(AF_INET6, sizeof(struct in6_addr),
+					    &addr, s1, sizeof(s1)));
+		} else if (RTA_PAYLOAD(i) >= sizeof(__be32)) {
+			__be32 addr = rta_getattr_u32(i);
+			fprintf(f, "        %s\n",
+				format_host(AF_INET, 4, &addr, s1, sizeof(s1)));
+		}
+	}
+}
+
 static void vxlan_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[])
 {
 	__u32 vni;
@@ -277,6 +359,9 @@ static void vxlan_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[])
 	if (tb[IFLA_VXLAN_LIMIT] &&
 	    (maxaddr = rta_getattr_u32(tb[IFLA_VXLAN_LIMIT]) != 0))
 		    fprintf(f, "maxaddr %u ", maxaddr);
+
+	if (tb[IFLA_VXLAN_REMOTE_LST])
+		vxlan_print_remotes(f, tb[IFLA_VXLAN_REMOTE_LST]);
 }
 
 struct link_util vxlan_link_util = {
-- 
1.8.1.5

Re: [PATCH net-next v2 2/2] vxlan: allow specifying multiple default destinations

[Cong Wang]

On Tue, 28 May 2013 at 08:31 GMT, Mike Rapoport <mike.rapoport@ravellosystems.com> wrote:
> +	IFLA_VXLAN_REMOTE_ADD,
> +	IFLA_VXLAN_REMOTE_DEL,
> +	IFLA_VXLAN_REMOTE_LST,

Usually we use NEW_* and GET_* instead of *_ADD and *_LST.

Re: [PATCH net-next v2 2/2] vxlan: allow specifying multiple default destinations

[Mike Rapoport]

On Tue, May 28, 2013 at 11:46 AM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>
> On Tue, 28 May 2013 at 08:31 GMT, Mike Rapoport <mike.rapoport@ravellosystems.com> wrote:
> > +     IFLA_VXLAN_REMOTE_ADD,
> > +     IFLA_VXLAN_REMOTE_DEL,
> > +     IFLA_VXLAN_REMOTE_LST,
>
> Usually we use NEW_* and GET_* instead of *_ADD and *_LST.

Semantically ADD and LST seem more correct to me. We are adding a
destination and asking for destinations list...

> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html




--
Sincerely yours,
Mike.

Re: [PATCH net-next v2 2/2] vxlan: allow specifying multiple default destinations

[Thomas Graf]

On 05/28/13 at 11:31am, Mike Rapoport wrote:
> +/* Add remote to default destinations list */
> +static int vxlan_remote_add(struct vxlan_dev *vxlan, struct nlattr *attr)
> +{
> +	struct nlattr *i;
> +	__be32 ip;
> +	__be16 port;
> +	u32 ifindex, vni;
> +	int rem, err = 0;
> +	bool addr_set = false;
> +
> +	port = vxlan->dst_port;
> +	vni = vxlan->default_dst.remote_vni;
> +	ifindex = vxlan->default_dst.remote_ifindex;
> +
> +	nla_for_each_nested(i, attr, rem) {
> +		switch (nla_type(i)) {
> +		case IFLA_VXLAN_REMOTE_ADDR:
> +			ip = nla_get_be32(i);
> +			addr_set = true;
> +			break;
> +		case IFLA_VXLAN_REMOTE_PORT:
> +			port = nla_get_be16(i);
> +			break;
> +		case IFLA_VXLAN_REMOTE_VNI:
> +			vni = nla_get_u32(i);
> +			break;
> +		case IFLA_VXLAN_REMOTE_IFINDEX:
> +			ifindex = nla_get_u32(i);
> +			break;
> +		default:
> +			err = -EINVAL;
> +			break;
> +		};
> +
> +		if (err)
> +			return err;
> +	}
> +
> +	if (!addr_set)
> +		return -EINVAL;
> +
> +	err = vxlan_rdst_append(&vxlan->default_dst, ip, port, vni, ifindex);
> +	if (err < 0)
> +		return err;

You must be seeing a warning about a possible use of uninitialized
data here. 'ip' could be uninitialized.

In fact you are using these Netlink attributes blindy without
validation. You need to extend vxlan_validate() and verify the
type and length of each attribute. User space is not guaranteed
to send the data you expect.

> +	netdev_dbg(vxlan->dev, "dstadd %pI4\n", &ip);

This doesn't look suitable for inclusion.

> @@ -1513,6 +1601,27 @@ static struct vxlan_sock *vxlan_socket_create(struct net *net, __be16 port)
>  	return vs;
>  }
>  
> +static int vxlan_changelink(struct net_device *dev,
> +			    struct nlattr *tb[], struct nlattr *data[])
> +{
> +	struct vxlan_dev *vxlan = netdev_priv(dev);
> +	int err;
> +
> +	if (data[IFLA_VXLAN_REMOTE_ADD]) {
> +		err = vxlan_remote_add(vxlan, data[IFLA_VXLAN_REMOTE_ADD]);
> +		if (err)
> +			return err;
> +	}
> +
> +	if (data[IFLA_VXLAN_REMOTE_DEL]) {
> +		err = vxlan_remote_delete(vxlan, data[IFLA_VXLAN_REMOTE_DEL]);
> +		if (err)
> +			return err;
> +	}

First of all please use NEW and DEL because that is what we use
everywhere in Netlink and everyone is used to it.

I would also make both of these a list of nested attribute to allow
for multiple additions and deletions in the future. And please use
the same format for NEW and DEL. Using a u32 directly for deletions
sets the format in stone and we can't extend it in the future.

Using attributes as you do in the addition case allows to support
IPv6 addresses in the future.

Why not make it even more generic and encode changes like this:

enum {
  IFLA_VXLAN_REMOTE_NEW,
  IFLA_VXLAN_REMOTE_DEL,
};

IFLA_VXLAN_REMOTES = {
  [IFLA_VXLAN_REMOTE_NEW] = {
    [IFLA_VXLAN_REMOTE_PORT] = ...,
    [IFLA_VXLAN_REMOTE_ADDR] = ...,
  },
  [IFLA_VXLAN_REMOTE_NEW] = {
    ...,
  },
  [IFLA_VXLAN_REMOTE_DEL] = {
    [IFLA_VXLAN_REMOTE_ADDR] = ...,
  },
}

> @@ -1707,6 +1838,24 @@ static int vxlan_fill_info(struct sk_buff *skb, const struct net_device *dev)
>  	if (nla_put(skb, IFLA_VXLAN_PORT_RANGE, sizeof(ports), &ports))
>  		goto nla_put_failure;
>  
> +	if (vxlan->remote_cnt) {
> +		struct vxlan_rdst *rdst;
> +		struct nlattr *nest;
> +
> +		nest = nla_nest_start(skb, IFLA_VXLAN_REMOTE_LST);
> +		if (nest == NULL)
> +			goto nla_put_failure;
> +
> +		for (rdst = vxlan->default_dst.remote_next; rdst;
> +		     rdst = rdst->remote_next) {
> +			__be32 ip = rdst->remote_ip;
> +			if (nla_put_be32(skb, IFLA_VXLAN_REMOTE_ADDR, ip))
> +				goto nla_put_failure;


Are you sure that you never want to extend this in the future? What
about providing IFLA_VXLAN_REMOTE_PORT in the future?

I suggest to encode it as:

IFLA_VXLAN_REMOTES = {
  [0] = {
    [IFLA_VXLAN_REMOTE_ADDR] = <ADDR1>
  },
  [1] = {
    [IFLA_VXLAN_REMOTE_ADDR] = <ADDR2>
  }
},

Allowing to extend it to further attributes in the future.

> +enum {
> +	IFLA_VXLAN_REMOTE_UNSPEC,
> +	IFLA_VXLAN_REMOTE_ADDR,
> +	IFLA_VXLAN_REMOTE_IFINDEX,
> +	IFLA_VXLAN_REMOTE_PORT,
> +	IFLA_VXLAN_REMOTE_VNI,
> +	__IFLA_VXLAN_REMOTE_MAX
> +};
> +
> +#define IFLA_VXLAN_REMOTE_MAX	(__IFLA_VXLAN_REMOTE_MAX - 1)

Each of these new attributes must have an struct nla_policy
entry that is enforced.

[PATCH iproute2] vxlan: allow specifying multiple default destinations

[Mike Rapoport]

Signed-off-by: Mike Rapoport <mike.rapoport@ravellosystems.com>
---
 ip/iplink_vxlan.c | 125 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 125 insertions(+)

diff --git a/ip/iplink_vxlan.c b/ip/iplink_vxlan.c
index 1025326..be6c0ac 100644
--- a/ip/iplink_vxlan.c
+++ b/ip/iplink_vxlan.c
@@ -28,11 +28,87 @@ static void explain(void)
 	fprintf(stderr, "                 [ port MIN MAX ] [ [no]learning ]\n");
 	fprintf(stderr, "                 [ [no]proxy ] [ [no]rsc ]\n");
 	fprintf(stderr, "                 [ [no]l2miss ] [ [no]l3miss ]\n");
+	fprintf(stderr, "                 [ dstadd DST ]\n");
+	fprintf(stderr, "                 [ dstdel ADDR ]\n");
 	fprintf(stderr, "\n");
 	fprintf(stderr, "Where: VNI := 0-16777215\n");
 	fprintf(stderr, "       ADDR := { IP_ADDRESS | any }\n");
 	fprintf(stderr, "       TOS  := { NUMBER | inherit }\n");
 	fprintf(stderr, "       TTL  := { 1..255 | inherit }\n");
+	fprintf(stderr, "       DST  := [ ADDR [port PORT] [vni VNI] [via DEV]]\n");
+}
+
+static int vxlan_parse_dstadd(int *argcp, char ***argvp, struct nlmsghdr *n)
+{
+	int argc = *argcp;
+	char **argv = *argvp;
+	__u32 vni, ifindex;
+	__u16 port;
+	struct rtattr *nest;
+	int addr_set = 0;
+
+	nest = addattr_nest(n, 1024, IFLA_VXLAN_REMOTE_NEW);
+
+	while (argc > 0) {
+		if (!matches(*argv, "vni")) {
+			NEXT_ARG();
+			if (get_u32(&vni, *argv, 0) ||
+			    vni >= 1u << 24)
+				invarg("invalid id", *argv);
+			addattr32(n, 1024, IFLA_VXLAN_REMOTE_VNI, vni);
+		} else if (!matches(*argv, "port")) {
+			NEXT_ARG();
+			if (get_u16(&port, *argv, 0))
+				invarg("port", *argv);
+			addattr32(n, 1024, IFLA_VXLAN_REMOTE_PORT, htons(port));
+		} else if (!matches(*argv, "via")) {
+			NEXT_ARG();
+			ifindex = if_nametoindex(*argv);
+			addattr32(n, 1024, IFLA_VXLAN_REMOTE_IFINDEX, ifindex);
+		} else {
+			inet_prefix addr;
+			get_prefix(&addr, *argv, AF_UNSPEC);
+			addattr_l(n, 1024, IFLA_VXLAN_REMOTE_ADDR,
+				  &addr.data, addr.bytelen);
+			addr_set = 1;
+		}
+		argc--, argv++;
+	}
+
+	if (!addr_set)
+		incomplete_command();
+
+	addattr_nest_end(n, nest);
+
+	*argcp = argc;
+	*argvp = argv;
+	return 0;
+}
+
+static int vxlan_parse_dstdel(int *argcp, char ***argvp, struct nlmsghdr *n)
+{
+	int argc = *argcp;
+	char **argv = *argvp;
+	struct rtattr *nest;
+
+	nest = addattr_nest(n, 1024, IFLA_VXLAN_REMOTE_DEL);
+
+	while (argc > 0) {
+		inet_prefix addr;
+		get_prefix(&addr, *argv, AF_UNSPEC);
+		addattr_l(n, 1024, IFLA_VXLAN_REMOTE_ADDR,
+			  &addr.data, addr.bytelen);
+		argc--, argv++;
+	}
+
+	if (argc == *argcp)
+		incomplete_command();
+
+	addattr_nest_end(n, nest);
+
+	*argcp = argc;
+	*argvp = argv;
+	return 0;
 }
 
 static int vxlan_parse_opt(struct link_util *lu, int argc, char **argv,
@@ -54,6 +130,7 @@ static int vxlan_parse_opt(struct link_util *lu, int argc, char **argv,
 	__u32 age = 0;
 	__u32 maxaddr = 0;
 	struct ifla_vxlan_port_range range = { 0, 0 };
+	struct rtattr *remotes;
 
 	while (argc > 0) {
 		if (!matches(*argv, "id") ||
@@ -125,6 +202,16 @@ static int vxlan_parse_opt(struct link_util *lu, int argc, char **argv,
 				invarg("max port", *argv);
 			range.low = htons(minport);
 			range.high = htons(maxport);
+		} else if (!matches(*argv, "dstadd")) {
+			NEXT_ARG();
+			remotes = addattr_nest(n, 1024, IFLA_VXLAN_REMOTES);
+			vxlan_parse_dstadd(&argc, &argv, n);
+			addattr_nest_end(n, remotes);
+		} else if (!matches(*argv, "dstdel")) {
+			NEXT_ARG();
+			remotes = addattr_nest(n, 1024, IFLA_VXLAN_REMOTES);
+			vxlan_parse_dstdel(&argc, &argv, n);
+			addattr_nest_end(n, remotes);
 		} else if (!matches(*argv, "nolearning")) {
 			learning = 0;
 		} else if (!matches(*argv, "learning")) {
@@ -187,6 +274,41 @@ static int vxlan_parse_opt(struct link_util *lu, int argc, char **argv,
 	return 0;
 }
 
+static void vxlan_print_remote(FILE *f, struct rtattr *attr)
+{
+	struct rtattr *tb[IFLA_VXLAN_REMOTE_MAX];
+	char s1[1024];
+
+	parse_rtattr_nested(tb, IFLA_VXLAN_REMOTE_MAX, attr);
+
+	if (tb[IFLA_VXLAN_REMOTE_ADDR]) {
+		struct rtattr *i = tb[IFLA_VXLAN_REMOTE_ADDR];
+		if (RTA_PAYLOAD(i) >= sizeof(struct in6_addr)) {
+			struct in6_addr addr;
+			memcpy(&addr, RTA_DATA(i), sizeof(struct in6_addr));
+			fprintf(f, "        %s\n",
+				format_host(AF_INET6, sizeof(struct in6_addr),
+					    &addr, s1, sizeof(s1)));
+		} else if (RTA_PAYLOAD(i) >= sizeof(__be32)) {
+			__be32 addr = rta_getattr_u32(i);
+			fprintf(f, "        %s\n",
+				format_host(AF_INET, 4, &addr, s1, sizeof(s1)));
+		}
+	}
+}
+
+static void vxlan_print_remotes(FILE *f, struct rtattr *attr)
+{
+	struct rtattr *i;
+	int rem, n = 0;
+
+	fprintf(f, "\n      default destinations :\n");
+
+	rem = RTA_PAYLOAD(attr);
+	for (i = RTA_DATA(attr); RTA_OK(i, rem); i = RTA_NEXT(i, rem), n++)
+		vxlan_print_remote(f, i);
+}
+
 static void vxlan_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[])
 {
 	__u32 vni;
@@ -277,6 +399,9 @@ static void vxlan_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[])
 	if (tb[IFLA_VXLAN_LIMIT] &&
 	    (maxaddr = rta_getattr_u32(tb[IFLA_VXLAN_LIMIT]) != 0))
 		    fprintf(f, "maxaddr %u ", maxaddr);
+
+	if (tb[IFLA_VXLAN_REMOTES])
+		vxlan_print_remotes(f, tb[IFLA_VXLAN_REMOTES]);
 }
 
 struct link_util vxlan_link_util = {
-- 
1.8.1.5

Re: [PATCH iproute2] vxlan: allow specifying multiple default destinations

[Cong Wang]

On Wed, 29 May 2013 at 10:00 GMT, Mike Rapoport <mike.rapoport@ravellosystems.com> wrote:
> Signed-off-by: Mike Rapoport <mike.rapoport@ravellosystems.com>
> ---
>  ip/iplink_vxlan.c | 125 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 125 insertions(+)
>
> diff --git a/ip/iplink_vxlan.c b/ip/iplink_vxlan.c
> index 1025326..be6c0ac 100644
> --- a/ip/iplink_vxlan.c
> +++ b/ip/iplink_vxlan.c
> @@ -28,11 +28,87 @@ static void explain(void)
>  	fprintf(stderr, "                 [ port MIN MAX ] [ [no]learning ]\n");
>  	fprintf(stderr, "                 [ [no]proxy ] [ [no]rsc ]\n");
>  	fprintf(stderr, "                 [ [no]l2miss ] [ [no]l3miss ]\n");
> +	fprintf(stderr, "                 [ dstadd DST ]\n");
> +	fprintf(stderr, "                 [ dstdel ADDR ]\n");

Excuse me, but this looks like a design failure as you manipulate
remotes with `ip link` while creating vxlan devices, shouldn't this be
in a standard alone tool if we can't reuse any existing tool? Or am I
missing anything?

Re: [PATCH iproute2] vxlan: allow specifying multiple default destinations

[Mike Rapoport]

On Wed, May 29, 2013 at 1:13 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On Wed, 29 May 2013 at 10:00 GMT, Mike Rapoport <mike.rapoport@ravellosystems.com> wrote:
>> Signed-off-by: Mike Rapoport <mike.rapoport@ravellosystems.com>
>> ---
>>  ip/iplink_vxlan.c | 125 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 125 insertions(+)
>>
>> diff --git a/ip/iplink_vxlan.c b/ip/iplink_vxlan.c
>> index 1025326..be6c0ac 100644
>> --- a/ip/iplink_vxlan.c
>> +++ b/ip/iplink_vxlan.c
>> @@ -28,11 +28,87 @@ static void explain(void)
>>       fprintf(stderr, "                 [ port MIN MAX ] [ [no]learning ]\n");
>>       fprintf(stderr, "                 [ [no]proxy ] [ [no]rsc ]\n");
>>       fprintf(stderr, "                 [ [no]l2miss ] [ [no]l3miss ]\n");
>> +     fprintf(stderr, "                 [ dstadd DST ]\n");
>> +     fprintf(stderr, "                 [ dstdel ADDR ]\n");
>
> Excuse me, but this looks like a design failure as you manipulate
> remotes with `ip link` while creating vxlan devices, shouldn't this be
> in a standard alone tool if we can't reuse any existing tool? Or am I
> missing anything?

Frankly, I had a long hesitation about the userspace implementation.
>From one side it seems very logical to use ip/iplink_vxlan for vxlan
device manipulations. Moreover, since the remotes are used pretty much
the same way as the group address, adding the remotes management to
ip/iplink_vxlan makes a lot of sense. Besides, creation of stand alone
tool for remote list manipulation in vxlan seemed to me little bit far
fetched.

On the other hand, I quite agree with you that
ip link add vxlan0 ... dstadd 192.168.1.1
or
ip link set vxlan0 ... dstdel 192.168.1.1
looks weird at least.

> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



--
Sincerely yours,
Mike.

Re: [PATCH iproute2] vxlan: allow specifying multiple default destinations

[Stephen Hemminger]

On Wed, 29 May 2013 13:52:55 +0300
Mike Rapoport <mike.rapoport@ravellosystems.com> wrote:

> On Wed, May 29, 2013 at 1:13 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > On Wed, 29 May 2013 at 10:00 GMT, Mike Rapoport <mike.rapoport@ravellosystems.com> wrote:
> >> Signed-off-by: Mike Rapoport <mike.rapoport@ravellosystems.com>
> >> ---
> >>  ip/iplink_vxlan.c | 125 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>  1 file changed, 125 insertions(+)
> >>
> >> diff --git a/ip/iplink_vxlan.c b/ip/iplink_vxlan.c
> >> index 1025326..be6c0ac 100644
> >> --- a/ip/iplink_vxlan.c
> >> +++ b/ip/iplink_vxlan.c
> >> @@ -28,11 +28,87 @@ static void explain(void)
> >>       fprintf(stderr, "                 [ port MIN MAX ] [ [no]learning ]\n");
> >>       fprintf(stderr, "                 [ [no]proxy ] [ [no]rsc ]\n");
> >>       fprintf(stderr, "                 [ [no]l2miss ] [ [no]l3miss ]\n");
> >> +     fprintf(stderr, "                 [ dstadd DST ]\n");
> >> +     fprintf(stderr, "                 [ dstdel ADDR ]\n");
> >
> > Excuse me, but this looks like a design failure as you manipulate
> > remotes with `ip link` while creating vxlan devices, shouldn't this be
> > in a standard alone tool if we can't reuse any existing tool? Or am I
> > missing anything?
> 
> Frankly, I had a long hesitation about the userspace implementation.
> From one side it seems very logical to use ip/iplink_vxlan for vxlan
> device manipulations. Moreover, since the remotes are used pretty much
> the same way as the group address, adding the remotes management to
> ip/iplink_vxlan makes a lot of sense. Besides, creation of stand alone
> tool for remote list manipulation in vxlan seemed to me little bit far
> fetched.
> 
> On the other hand, I quite agree with you that
> ip link add vxlan0 ... dstadd 192.168.1.1
> or
> ip link set vxlan0 ... dstdel 192.168.1.1
> looks weird at least.

Don't like add/delete semantics here either.
Maybe replace or modify, or has this grown enough that having its own
command line tool "vxlan ..." makes sense?

Re: [PATCH iproute2] vxlan: allow specifying multiple default destinations

[Mike Rapoport]

On Thu, May 30, 2013 at 1:56 AM, Stephen Hemminger
<stephen@networkplumber.org> wrote:
> On Wed, 29 May 2013 13:52:55 +0300
> Mike Rapoport <mike.rapoport@ravellosystems.com> wrote:
>
>> On Wed, May 29, 2013 at 1:13 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>> > On Wed, 29 May 2013 at 10:00 GMT, Mike Rapoport <mike.rapoport@ravellosystems.com> wrote:
>> >> Signed-off-by: Mike Rapoport <mike.rapoport@ravellosystems.com>
>> >> ---
>> >>  ip/iplink_vxlan.c | 125 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> >>  1 file changed, 125 insertions(+)
>> >>
>> >> diff --git a/ip/iplink_vxlan.c b/ip/iplink_vxlan.c
>> >> index 1025326..be6c0ac 100644
>> >> --- a/ip/iplink_vxlan.c
>> >> +++ b/ip/iplink_vxlan.c
>> >> @@ -28,11 +28,87 @@ static void explain(void)
>> >>       fprintf(stderr, "                 [ port MIN MAX ] [ [no]learning ]\n");
>> >>       fprintf(stderr, "                 [ [no]proxy ] [ [no]rsc ]\n");
>> >>       fprintf(stderr, "                 [ [no]l2miss ] [ [no]l3miss ]\n");
>> >> +     fprintf(stderr, "                 [ dstadd DST ]\n");
>> >> +     fprintf(stderr, "                 [ dstdel ADDR ]\n");
>> >
>> > Excuse me, but this looks like a design failure as you manipulate
>> > remotes with `ip link` while creating vxlan devices, shouldn't this be
>> > in a standard alone tool if we can't reuse any existing tool? Or am I
>> > missing anything?
>>
>> Frankly, I had a long hesitation about the userspace implementation.
>> From one side it seems very logical to use ip/iplink_vxlan for vxlan
>> device manipulations. Moreover, since the remotes are used pretty much
>> the same way as the group address, adding the remotes management to
>> ip/iplink_vxlan makes a lot of sense. Besides, creation of stand alone
>> tool for remote list manipulation in vxlan seemed to me little bit far
>> fetched.
>>
>> On the other hand, I quite agree with you that
>> ip link add vxlan0 ... dstadd 192.168.1.1
>> or
>> ip link set vxlan0 ... dstdel 192.168.1.1
>> looks weird at least.
>
> Don't like add/delete semantics here either.
> Maybe replace or modify,

I think that replace or modify do not express the actual operation
meaning. My intention with dstadd was "add remote host X to
pseudo-multicast group". Replace/modify maybe nice to have features to
avoid doing delete+ add.

> or has this grown enough that having its own
> command line tool "vxlan ..." makes sense?

Say, misc/vxlan that will handle remote destinations management? Or
should it take care of some vxlan parameters currently implemented in
ip/iplink_vxlan and bridge/fdb?

--
Sincerely yours,
Mike.

Re: [PATCH iproute2] vxlan: allow specifying multiple default destinations

[Thomas Graf]

On 05/30/13 at 11:42am, Mike Rapoport wrote:
> On Thu, May 30, 2013 at 1:56 AM, Stephen Hemminger
> <stephen@networkplumber.org> wrote:
> > On Wed, 29 May 2013 13:52:55 +0300
> > Mike Rapoport <mike.rapoport@ravellosystems.com> wrote:
> >> Frankly, I had a long hesitation about the userspace implementation.
> >> From one side it seems very logical to use ip/iplink_vxlan for vxlan
> >> device manipulations. Moreover, since the remotes are used pretty much
> >> the same way as the group address, adding the remotes management to
> >> ip/iplink_vxlan makes a lot of sense. Besides, creation of stand alone
> >> tool for remote list manipulation in vxlan seemed to me little bit far
> >> fetched.
> >>
> >> On the other hand, I quite agree with you that
> >> ip link add vxlan0 ... dstadd 192.168.1.1
> >> or
> >> ip link set vxlan0 ... dstdel 192.168.1.1
> >> looks weird at least.
> >
> > Don't like add/delete semantics here either.
> > Maybe replace or modify,
> 
> I think that replace or modify do not express the actual operation
> meaning. My intention with dstadd was "add remote host X to
> pseudo-multicast group". Replace/modify maybe nice to have features to
> avoid doing delete+ add.

The alternative would be to require iproute2 to always provide the
full list of remote addresses like we do we route nexthops.

I do like the add/del though and don't see a problem with requiring
an ''ip link set [..] dstadd/dstdel''

> > or has this grown enough that having its own
> > command line tool "vxlan ..." makes sense?
> 
> Say, misc/vxlan that will handle remote destinations management? Or
> should it take care of some vxlan parameters currently implemented in
> ip/iplink_vxlan and bridge/fdb?

What do we gain from a separate tool?

Re: [PATCH iproute2] vxlan: allow specifying multiple default destinations

[Mike Rapoport]

On Thu, May 30, 2013 at 12:44:24PM +0100, Thomas Graf wrote:
> On 05/30/13 at 11:42am, Mike Rapoport wrote:
> > On Thu, May 30, 2013 at 1:56 AM, Stephen Hemminger
> > <stephen@networkplumber.org> wrote:
> > > On Wed, 29 May 2013 13:52:55 +0300
> > > Mike Rapoport <mike.rapoport@ravellosystems.com> wrote:
> > >> Frankly, I had a long hesitation about the userspace implementation.
> > >> From one side it seems very logical to use ip/iplink_vxlan for vxlan
> > >> device manipulations. Moreover, since the remotes are used pretty much
> > >> the same way as the group address, adding the remotes management to
> > >> ip/iplink_vxlan makes a lot of sense. Besides, creation of stand alone
> > >> tool for remote list manipulation in vxlan seemed to me little bit far
> > >> fetched.
> > >>
> > >> On the other hand, I quite agree with you that
> > >> ip link add vxlan0 ... dstadd 192.168.1.1
> > >> or
> > >> ip link set vxlan0 ... dstdel 192.168.1.1
> > >> looks weird at least.
> > >
> > > Don't like add/delete semantics here either.
> > > Maybe replace or modify,
> > 
> > I think that replace or modify do not express the actual operation
> > meaning. My intention with dstadd was "add remote host X to
> > pseudo-multicast group". Replace/modify maybe nice to have features to
> > avoid doing delete+ add.
> 
> The alternative would be to require iproute2 to always provide the
> full list of remote addresses like we do we route nexthops.
> 
> I do like the add/del though and don't see a problem with requiring
> an ''ip link set [..] dstadd/dstdel''

I'm feeling Ok about "ip link set [..] dstadd/dstdel". What does bother
me is that you can't have different parameters for "ip link add" and "ip
link set" for vxlan (and other iplink) utility. So, one can use
ip link add [..] dstdel
which does not make sense...

> > > or has this grown enough that having its own
> > > command line tool "vxlan ..." makes sense?
> > 
> > Say, misc/vxlan that will handle remote destinations management? Or
> > should it take care of some vxlan parameters currently implemented in
> > ip/iplink_vxlan and bridge/fdb?
> 
> What do we gain from a separate tool?

--
Sincerely yours,
Mike.

Re: [PATCH iproute2] vxlan: allow specifying multiple default destinations

[Thomas Graf]

On 05/30/13 at 03:46pm, Mike Rapoport wrote:
> I'm feeling Ok about "ip link set [..] dstadd/dstdel". What does bother
> me is that you can't have different parameters for "ip link add" and "ip
> link set" for vxlan (and other iplink) utility. So, one can use
> ip link add [..] dstdel
> which does not make sense...

You can easily pass an additional argument into iplink_modify()
and exclude certain options in the "add" use case.

Re: [PATCH iproute2] vxlan: allow specifying multiple default destinations

[Stephen Hemminger]

On Thu, 30 May 2013 12:44:24 +0100
Thomas Graf <tgraf@suug.ch> wrote:

> > Say, misc/vxlan that will handle remote destinations management? Or
> > should it take care of some vxlan parameters currently implemented in
> > ip/iplink_vxlan and bridge/fdb?  
> 
> What do we gain from a separate tool?

At some point the syntax becomes unwieldy, could even just be a shell script.

Re: [PATCH iproute2] vxlan: allow specifying multiple default destinations

[Mike Rapoport]

On Thu, May 30, 2013 at 6:57 PM, Thomas Graf <tgraf@suug.ch> wrote:
> On 05/30/13 at 03:46pm, Mike Rapoport wrote:
>> I'm feeling Ok about "ip link set [..] dstadd/dstdel". What does bother
>> me is that you can't have different parameters for "ip link add" and "ip
>> link set" for vxlan (and other iplink) utility. So, one can use
>> ip link add [..] dstdel
>> which does not make sense...
>
> You can easily pass an additional argument into iplink_modify()
> and exclude certain options in the "add" use case.

I think there's no need to pass an additional argument to iplink_modify.
The vxlan_parse_opts may check the flags in nlmsghdr to distinguish
between the "add" and "set" cases.
Than we'll have 'ip link add [..]' as it was and the 'ip link set
[..]' will be used to manage default destinations.

--
Sincerely yours,
Mike.

Re: [PATCH iproute2] vxlan: allow specifying multiple default destinations

[Stephen Hemminger]

On Sun, 2 Jun 2013 10:09:23 +0300
Mike Rapoport <mike.rapoport@ravellosystems.com> wrote:

> On Thu, May 30, 2013 at 6:57 PM, Thomas Graf <tgraf@suug.ch> wrote:
> > On 05/30/13 at 03:46pm, Mike Rapoport wrote:
> >> I'm feeling Ok about "ip link set [..] dstadd/dstdel". What does bother
> >> me is that you can't have different parameters for "ip link add" and "ip
> >> link set" for vxlan (and other iplink) utility. So, one can use
> >> ip link add [..] dstdel
> >> which does not make sense...
> >
> > You can easily pass an additional argument into iplink_modify()
> > and exclude certain options in the "add" use case.
> 
> I think there's no need to pass an additional argument to iplink_modify.
> The vxlan_parse_opts may check the flags in nlmsghdr to distinguish
> between the "add" and "set" cases.
> Than we'll have 'ip link add [..]' as it was and the 'ip link set
> [..]' will be used to manage default destinations.
> 
> --
> Sincerely yours,
> Mike.


I think multiple destinations should be handled like multipath routes.
I.e you don't specify multiple destinations on the command line, you specify them
individually and can add/delete them

If you delete the last destination then the forwarding entry should disappear.
The collapsing of multiple entries into one entry in table is an internal data structure
choice of vxlan and shouldn't be part of the netlink API requirement.

The API to iproute2/netlink should look like routing (through bridge fdb command).
Feel free to reject this if since I don't actually use this stuff.

Re: [PATCH iproute2] vxlan: allow specifying multiple default destinations

[Mike Rapoport]

On Wed, Jun 5, 2013 at 7:30 AM, Stephen Hemminger
<stephen@networkplumber.org> wrote:
> On Sun, 2 Jun 2013 10:09:23 +0300
> Mike Rapoport <mike.rapoport@ravellosystems.com> wrote:
>
>> On Thu, May 30, 2013 at 6:57 PM, Thomas Graf <tgraf@suug.ch> wrote:
>> > On 05/30/13 at 03:46pm, Mike Rapoport wrote:
>> >> I'm feeling Ok about "ip link set [..] dstadd/dstdel". What does bother
>> >> me is that you can't have different parameters for "ip link add" and "ip
>> >> link set" for vxlan (and other iplink) utility. So, one can use
>> >> ip link add [..] dstdel
>> >> which does not make sense...
>> >
>> > You can easily pass an additional argument into iplink_modify()
>> > and exclude certain options in the "add" use case.
>>
>> I think there's no need to pass an additional argument to iplink_modify.
>> The vxlan_parse_opts may check the flags in nlmsghdr to distinguish
>> between the "add" and "set" cases.
>> Than we'll have 'ip link add [..]' as it was and the 'ip link set
>> [..]' will be used to manage default destinations.
>>
>> --
>> Sincerely yours,
>> Mike.
>
>
> I think multiple destinations should be handled like multipath routes.
> I.e you don't specify multiple destinations on the command line, you specify them
> individually and can add/delete them
>
> If you delete the last destination then the forwarding entry should disappear.
> The collapsing of multiple entries into one entry in table is an internal data structure
> choice of vxlan and shouldn't be part of the netlink API requirement.
>
> The API to iproute2/netlink should look like routing (through bridge fdb command).
> Feel free to reject this if since I don't actually use this stuff.

Well, if we're to follow David Stevens suggestion to make default
destination fdb entry with ALL_ZEROS_MAC (1), they surely can be
managed using 'bridge fdb'.

(1) http://thread.gmane.org/gmane.linux.network/270969/focus=271791

--
Sincerely yours,
Mike.

Re: [PATCH iproute2] vxlan: allow specifying multiple default destinations

[Tomasz Chmielewski]

> Signed-off-by: Mike Rapoport <mike.rapoport@ravellosystems.com>
> ---
> This patch depends on the pending changes to ip/iplink_vxlan.c as as
> well as on IPv6 support in vxlan. I'll rebase and resend it once all
> the changes to vxlan are merged.

Was this one (and related) ever merged?

Full thread here:

http://marc.info/?t=136688790500006&r=1&w=4



Tomasz Chmielewski
https://lxadm.com