[PATCH v3 0/3] iproute2: support for device groups

[PATCH v3 0/3] iproute2: support for device groups

[Vlad Dogaru]

This patch series adds userspace support for network device groups.
There is support for setting device groups, listing only interfaces of a
specific group, and setting basic device parameters for all interfaces
in a group.

Changes since version 2:
 * groups now have names. The mapping between names and numbers (which
   are used by the kernel) is in /etc/iproute2/group_map.
 * the ip man page and usage display function have been updated to
   reflect the new functionality.

Changes since version 1:
 * a single attribute is used for both setting the group of a device and
   manipulating an entire group.


Vlad Dogaru (3):
  iproute2: add support for setting device groups
  iproute2: support listing devices by group
  iproute2: support setting device parameters by group

 etc/iproute2/group_map    |    2 +
 include/linux/if_link.h   |    1 +
 include/linux/netdevice.h |    2 +-
 include/utils.h           |    5 +++-
 ip/ip_common.h            |    2 +
 ip/ipaddress.c            |   14 ++++++++++
 ip/iplink.c               |   60 ++++++++++++++++++++++++++++++++++++++++++---
 ip/link_veth.c            |    3 +-
 lib/utils.c               |   41 ++++++++++++++++++++++++++++++
 man/man8/ip.8             |   31 ++++++++++++++++++++---
 tc/m_ematch.c             |   39 -----------------------------
 11 files changed, 150 insertions(+), 50 deletions(-)
 create mode 100644 etc/iproute2/group_map

[PATCH v3 1/3] iproute2: add support for setting device groups

[Vlad Dogaru]

Use the group keyword to specify what group the device should belong to.
Since the kernel uses numbers internally, mapping of group names to
numbers is defined in /etc/iproute2/group_map. Example usage:

  ip link set dev eth0 group default

Signed-off-by: Vlad Dogaru <ddvlad@rosedu.org>
---
 etc/iproute2/group_map  |    2 ++
 include/linux/if_link.h |    1 +
 include/utils.h         |    2 ++
 ip/ip_common.h          |    2 ++
 ip/iplink.c             |    9 +++++++++
 lib/utils.c             |   41 +++++++++++++++++++++++++++++++++++++++++
 man/man8/ip.8           |    9 +++++++++
 tc/m_ematch.c           |   39 ---------------------------------------
 8 files changed, 66 insertions(+), 39 deletions(-)
 create mode 100644 etc/iproute2/group_map

diff --git a/etc/iproute2/group_map b/etc/iproute2/group_map
new file mode 100644
index 0000000..6f000b2
--- /dev/null
+++ b/etc/iproute2/group_map
@@ -0,0 +1,2 @@
+# device group names
+0	default
diff --git a/include/linux/if_link.h b/include/linux/if_link.h
index e87456c..54d05f9 100644
--- a/include/linux/if_link.h
+++ b/include/linux/if_link.h
@@ -135,6 +135,7 @@ enum {
 	IFLA_VF_PORTS,
 	IFLA_PORT_SELF,
 	IFLA_AF_SPEC,
+	IFLA_GROUP,
 	__IFLA_MAX
 };
 
diff --git a/include/utils.h b/include/utils.h
index 3da6998..327373e 100644
--- a/include/utils.h
+++ b/include/utils.h
@@ -152,4 +152,6 @@ extern int makeargs(char *line, char *argv[], int maxargs);
 struct iplink_req;
 int iplink_parse(int argc, char **argv, struct iplink_req *req,
 		char **name, char **type, char **link, char **dev);
+
+int lookup_map_id(const char *kind, int *dst, const char *file);
 #endif /* __UTILS_H__ */
diff --git a/ip/ip_common.h b/ip/ip_common.h
index a114186..b751d46 100644
--- a/ip/ip_common.h
+++ b/ip/ip_common.h
@@ -68,3 +68,5 @@ struct link_util *get_link_kind(const char *kind);
 #ifndef	INFINITY_LIFE_TIME
 #define     INFINITY_LIFE_TIME      0xFFFFFFFFU
 #endif
+
+#define GROUP_MAP "/etc/iproute2/group_map"
diff --git a/ip/iplink.c b/ip/iplink.c
index cb2c4f5..6c9df43 100644
--- a/ip/iplink.c
+++ b/ip/iplink.c
@@ -68,6 +68,7 @@ void iplink_usage(void)
 	fprintf(stderr, "	                  [ mtu MTU ]\n");
 	fprintf(stderr, "	                  [ netns PID ]\n");
 	fprintf(stderr, "			  [ alias NAME ]\n");
+	fprintf(stderr, "			  [ group GROUP ]\n");
 	fprintf(stderr, "	                  [ vf NUM [ mac LLADDR ]\n");
 	fprintf(stderr, "				   [ vlan VLANID [ qos VLAN-QOS ] ]\n");
 	fprintf(stderr, "				   [ rate TXRATE ] ] \n");
@@ -252,6 +253,7 @@ int iplink_parse(int argc, char **argv, struct iplink_req *req,
 	int mtu = -1;
 	int netns = -1;
 	int vf = -1;
+	int group = -1;
 
 	ret = argc;
 
@@ -297,6 +299,13 @@ int iplink_parse(int argc, char **argv, struct iplink_req *req,
 			if (get_integer(&mtu, *argv, 0))
 				invarg("Invalid \"mtu\" value\n", *argv);
 			addattr_l(&req->n, sizeof(*req), IFLA_MTU, &mtu, 4);
+		} else if (strcmp(*argv, "group") == 0) {
+			NEXT_ARG();
+			if (group != -1)
+				duparg("group", *argv);
+			if (lookup_map_id(*argv, &group, GROUP_MAP))
+				invarg("Invalid \"group\" value\n", *argv);
+			addattr_l(&req->n, sizeof(*req), IFLA_GROUP, &group, 4);
                 } else if (strcmp(*argv, "netns") == 0) {
                         NEXT_ARG();
                         if (netns != -1)
diff --git a/lib/utils.c b/lib/utils.c
index a60d884..3642cb7 100644
--- a/lib/utils.c
+++ b/lib/utils.c
@@ -25,6 +25,7 @@
 #include <linux/pkt_sched.h>
 #include <time.h>
 #include <sys/time.h>
+#include <errno.h>
 
 
 #include "utils.h"
@@ -760,3 +761,43 @@ int makeargs(char *line, char *argv[], int maxargs)
 
 	return argc;
 }
+
+int lookup_map_id(const char *kind, int *dst, const char *file)
+{
+	int err = -EINVAL;
+	char buf[512];
+	FILE *fd = fopen(file, "r");
+
+	if (fd == NULL) {
+		fprintf(stderr, "open %s: %s\n", file, strerror(errno));
+		return -errno;
+	}
+
+	while (fgets(buf, sizeof(buf), fd)) {
+		char namebuf[512], *p = buf;
+		int id;
+
+		while (*p == ' ' || *p == '\t')
+			p++;
+		if (*p == '#' || *p == '\n' || *p == 0)
+			continue;
+
+		if (sscanf(p, "%d %s", &id, namebuf) != 2) {
+			fprintf(stderr, "map %s corrupted at %s\n",
+			    file, p);
+			goto out;
+		}
+
+		if (!strcasecmp(namebuf, kind)) {
+			if (dst)
+				*dst = id;
+			err = 0;
+			goto out;
+		}
+	}
+
+	err = -ENOENT;
+out:
+	fclose(fd);
+	return err;
+}
diff --git a/man/man8/ip.8 b/man/man8/ip.8
index 8d55fa9..77e03d8 100644
--- a/man/man8/ip.8
+++ b/man/man8/ip.8
@@ -86,6 +86,9 @@ ip \- show / manipulate routing, devices, policy routing and tunnels
 .B alias
 .IR NAME  " |"
 .br
+.B  group
+.IR GROUP " |"
+.br
 .B vf
 .IR NUM " ["
 .B  mac
@@ -994,6 +997,12 @@ move the device to the network namespace associated with the process
 give the device a symbolic name for easy reference.
 
 .TP
+.BI group " GROUP"
+specify the group the device belongs to.
+The available groups are listed in file
+.BR "/etc/iproute2/group_map" .
+
+.TP
 .BI vf " NUM"
 specify a Virtual Function device to be configured. The associated PF device
 must be specified using the
diff --git a/tc/m_ematch.c b/tc/m_ematch.c
index 4c3acf8..4a6855c 100644
--- a/tc/m_ematch.c
+++ b/tc/m_ematch.c
@@ -87,45 +87,6 @@ out:
 	return err;
 }
 
-static int lookup_map_id(char *kind, int *dst, const char *file)
-{
-	int err = -EINVAL;
-	char buf[512];
-	FILE *fd = fopen(file, "r");
-
-	if (fd == NULL)
-		return -errno;
-
-	while (fgets(buf, sizeof(buf), fd)) {
-		char namebuf[512], *p = buf;
-		int id;
-
-		while (*p == ' ' || *p == '\t')
-			p++;
-		if (*p == '#' || *p == '\n' || *p == 0)
-			continue;
-
-		if (sscanf(p, "%d %s", &id, namebuf) != 2) {
-			fprintf(stderr, "ematch map %s corrupted at %s\n",
-			    file, p);
-			goto out;
-		}
-
-		if (!strcasecmp(namebuf, kind)) {
-			if (dst)
-				*dst = id;
-			err = 0;
-			goto out;
-		}
-	}
-
-	err = -ENOENT;
-	*dst = 0;
-out:
-	fclose(fd);
-	return err;
-}
-
 static struct ematch_util *get_ematch_kind(char *kind)
 {
 	static void *body;
-- 
1.7.1

[PATCH v3 2/3] iproute2: support listing devices by group

[Vlad Dogaru]

User can specify device group to list by using the devgroup keyword:

	ip link lst devgroup test

If no group is specified, 0 (default) is implied.

Signed-off-by: Vlad Dogaru <ddvlad@rosedu.org>
---
 include/linux/netdevice.h |    2 +-
 ip/ipaddress.c            |   14 ++++++++++++++
 ip/iplink.c               |    3 ++-
 man/man8/ip.8             |   11 +++++++++--
 4 files changed, 26 insertions(+), 4 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index bec4e23..ad2e34d 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -33,7 +33,7 @@
 
 #define MAX_ADDR_LEN	32		/* Largest hardware address length */
 
-
+#define INIT_NETDEV_GROUP	0	/* Initial group net devices belong to */
 
 /* Media selection options. */
 enum {
diff --git a/ip/ipaddress.c b/ip/ipaddress.c
index a775ecd..c634391 100644
--- a/ip/ipaddress.c
+++ b/ip/ipaddress.c
@@ -49,6 +49,7 @@ static struct
 	char *flushb;
 	int flushp;
 	int flushe;
+	int group;
 } filter;
 
 static int do_link;
@@ -246,6 +247,12 @@ int print_linkinfo(const struct sockaddr_nl *who,
 	    fnmatch(filter.label, RTA_DATA(tb[IFLA_IFNAME]), 0))
 		return 0;
 
+	if (tb[IFLA_GROUP]) {
+		int group = *(int*)RTA_DATA(tb[IFLA_GROUP]);
+		if (group != filter.group)
+			return -1;
+	}
+
 	if (n->nlmsg_type == RTM_DELLINK)
 		fprintf(fp, "Deleted ");
 
@@ -718,9 +725,12 @@ static int ipaddr_list_or_flush(int argc, char **argv, int flush)
 	if (filter.family == AF_UNSPEC)
 		filter.family = preferred_family;
 
+	filter.group = INIT_NETDEV_GROUP;
+
 	if (flush) {
 		if (argc <= 0) {
 			fprintf(stderr, "Flush requires arguments.\n");
+
 			return -1;
 		}
 		if (filter.family == AF_PACKET) {
@@ -779,6 +789,10 @@ static int ipaddr_list_or_flush(int argc, char **argv, int flush)
 		} else if (strcmp(*argv, "label") == 0) {
 			NEXT_ARG();
 			filter.label = *argv;
+		} else if (strcmp(*argv, "devgroup") == 0) {
+			NEXT_ARG();
+			if (lookup_map_id(*argv, &filter.group, GROUP_MAP))
+				invarg("Invalid \"group\" value\n", *argv);
 		} else {
 			if (strcmp(*argv, "dev") == 0) {
 				NEXT_ARG();
diff --git a/ip/iplink.c b/ip/iplink.c
index 6c9df43..a781848 100644
--- a/ip/iplink.c
+++ b/ip/iplink.c
@@ -66,13 +66,14 @@ void iplink_usage(void)
 	fprintf(stderr, "	                  [ address LLADDR ]\n");
 	fprintf(stderr, "	                  [ broadcast LLADDR ]\n");
 	fprintf(stderr, "	                  [ mtu MTU ]\n");
+	fprintf(stderr, "	                  [ group GROUP ]\n");
 	fprintf(stderr, "	                  [ netns PID ]\n");
 	fprintf(stderr, "			  [ alias NAME ]\n");
 	fprintf(stderr, "			  [ group GROUP ]\n");
 	fprintf(stderr, "	                  [ vf NUM [ mac LLADDR ]\n");
 	fprintf(stderr, "				   [ vlan VLANID [ qos VLAN-QOS ] ]\n");
 	fprintf(stderr, "				   [ rate TXRATE ] ] \n");
-	fprintf(stderr, "       ip link show [ DEVICE ]\n");
+	fprintf(stderr, "       ip link show [ DEVICE | devgroup DEVGROUP ]\n");
 
 	if (iplink_have_newlink()) {
 		fprintf(stderr, "\n");
diff --git a/man/man8/ip.8 b/man/man8/ip.8
index 77e03d8..5c42156 100644
--- a/man/man8/ip.8
+++ b/man/man8/ip.8
@@ -102,7 +102,9 @@ ip \- show / manipulate routing, devices, policy routing and tunnels
 
 .ti -8
 .B ip link show
-.RI "[ " DEVICE " ]"
+.RI "[ " DEVICE " | "
+.B devgroup
+.IR GROUP " ]"
 
 .ti -8
 .BR "ip addr" " { " add " | " del " } "
@@ -1065,7 +1067,12 @@ call.
 .BI dev " NAME " (default)
 .I NAME
 specifies the network device to show.
-If this argument is omitted all devices are listed.
+If this argument is omitted all devices in the default group are listed.
+
+.TP
+.BI devgroup " GROUP "
+.I GROUP
+specifies what group of devices to show.
 
 .TP
 .B up
-- 
1.7.1

[PATCH v3 3/3] iproute2: support setting device parameters by group

[Vlad Dogaru]

Users can now modify basic device parameters with a single call. We use
the devgroup keyword to specify the device group to work on. For
instance, to take down all interfaces in group `default':

	ip link set down devgroup default

Signed-off-by: Vlad Dogaru <ddvlad@rosedu.org>
---
 include/utils.h |    3 ++-
 ip/iplink.c     |   48 +++++++++++++++++++++++++++++++++++++++++++++---
 ip/link_veth.c  |    3 ++-
 man/man8/ip.8   |   11 +++++++++--
 4 files changed, 58 insertions(+), 7 deletions(-)

diff --git a/include/utils.h b/include/utils.h
index 327373e..c708b74 100644
--- a/include/utils.h
+++ b/include/utils.h
@@ -151,7 +151,8 @@ extern int makeargs(char *line, char *argv[], int maxargs);
 
 struct iplink_req;
 int iplink_parse(int argc, char **argv, struct iplink_req *req,
-		char **name, char **type, char **link, char **dev);
+		char **name, char **type, char **link, char **dev,
+		int *devgroup);
 
 int lookup_map_id(const char *kind, int *dst, const char *file);
 #endif /* __UTILS_H__ */
diff --git a/ip/iplink.c b/ip/iplink.c
index a781848..85d2efa 100644
--- a/ip/iplink.c
+++ b/ip/iplink.c
@@ -51,7 +51,7 @@ void iplink_usage(void)
 		fprintf(stderr, "                   type TYPE [ ARGS ]\n");
 		fprintf(stderr, "       ip link delete DEV type TYPE [ ARGS ]\n");
 		fprintf(stderr, "\n");
-		fprintf(stderr, "       ip link set DEVICE [ { up | down } ]\n");
+		fprintf(stderr, "       ip link set { dev DEVICE | devgroup DEVGROUP } [ { up | down } ]\n");
 	} else
 		fprintf(stderr, "Usage: ip link set DEVICE [ { up | down } ]\n");
 
@@ -246,7 +246,8 @@ int iplink_parse_vf(int vf, int *argcp, char ***argvp,
 
 
 int iplink_parse(int argc, char **argv, struct iplink_req *req,
-		char **name, char **type, char **link, char **dev)
+		char **name, char **type, char **link, char **dev,
+		int *devgroup)
 {
 	int ret, len;
 	char abuf[32];
@@ -257,6 +258,7 @@ int iplink_parse(int argc, char **argv, struct iplink_req *req,
 	int group = -1;
 
 	ret = argc;
+	*devgroup = -1; /* Not set. */
 
 	while (argc > 0) {
 		if (strcmp(*argv, "up") == 0) {
@@ -396,6 +398,20 @@ int iplink_parse(int argc, char **argv, struct iplink_req *req,
 		} else {
 			if (strcmp(*argv, "dev") == 0) {
 				NEXT_ARG();
+				if (*dev)
+					duparg2("dev", *argv);
+				*dev = *argv;
+				argc--; argv++;
+				continue;
+			}
+			if (matches(*argv, "devgroup") == 0) {
+				NEXT_ARG();
+				if (*devgroup != -1)
+					duparg("devgroup", *argv);
+				if (lookup_map_id(*argv, devgroup, GROUP_MAP))
+					invarg("Invalid \"devgroup\" value\n", *argv);
+				argc--; argv++;
+				continue;
 			}
 			if (matches(*argv, "help") == 0)
 				usage();
@@ -406,6 +422,11 @@ int iplink_parse(int argc, char **argv, struct iplink_req *req,
 		argc--; argv++;
 	}
 
+	if (*dev && (*devgroup != -1)) {
+		fprintf(stderr, "dev and devgroup cannot be both be present.\n");
+		exit(-1);
+	}
+
 	return ret - argc;
 }
 
@@ -416,6 +437,7 @@ static int iplink_modify(int cmd, unsigned int flags, int argc, char **argv)
 	char *name = NULL;
 	char *link = NULL;
 	char *type = NULL;
+	int devgroup;
 	struct link_util *lu = NULL;
 	struct iplink_req req;
 	int ret;
@@ -427,12 +449,32 @@ static int iplink_modify(int cmd, unsigned int flags, int argc, char **argv)
 	req.n.nlmsg_type = cmd;
 	req.i.ifi_family = preferred_family;
 
-	ret = iplink_parse(argc, argv, &req, &name, &type, &link, &dev);
+	ret = iplink_parse(argc, argv, &req, &name, &type, &link, &dev, &devgroup);
 	if (ret < 0)
 		return ret;
 
 	argc -= ret;
 	argv += ret;
+
+	if (devgroup != -1) {
+		if (argc) {
+			fprintf(stderr, "Garbage instead of arguments \"%s ...\". "
+					"Try \"ip link help\".\n", *argv);
+			return -1;
+		}
+		if (flags & NLM_F_CREATE) {
+			fprintf(stderr, "devgroup cannot be used when "
+					"creating devices.\n");
+			return -1;
+		}
+
+		req.i.ifi_index = 0;
+		addattr32(&req.n, sizeof(req), IFLA_GROUP, devgroup);
+		if (rtnl_talk(&rth, &req.n, 0, 0, NULL, NULL, NULL) < 0)
+			exit(2);
+		return 0;
+	}
+
 	ll_init_map(&rth);
 
 	if (type) {
diff --git a/ip/link_veth.c b/ip/link_veth.c
index 9f5e871..06974e7 100644
--- a/ip/link_veth.c
+++ b/ip/link_veth.c
@@ -30,6 +30,7 @@ static int veth_parse_opt(struct link_util *lu, int argc, char **argv,
 	char *name, *type, *link, *dev;
 	int err, len;
 	struct rtattr * data;
+	int devgroup;
 
 	if (strcmp(argv[0], "peer") != 0) {
 		usage();
@@ -42,7 +43,7 @@ static int veth_parse_opt(struct link_util *lu, int argc, char **argv,
 	hdr->nlmsg_len += sizeof(struct ifinfomsg);
 
 	err = iplink_parse(argc - 1, argv + 1, (struct iplink_req *)hdr,
-			   &name, &type, &link, &dev);
+			   &name, &type, &link, &dev, &devgroup);
 	if (err < 0)
 		return err;
 
diff --git a/man/man8/ip.8 b/man/man8/ip.8
index 5c42156..302523a 100644
--- a/man/man8/ip.8
+++ b/man/man8/ip.8
@@ -55,8 +55,10 @@ ip \- show / manipulate routing, devices, policy routing and tunnels
 .RI "[ " ARGS " ]"
 
 .ti -8
-.BI "ip link set " DEVICE
-.RB "{ " up " | " down " | " arp " { " on " | " off " } |"
+.BR "ip link set " {
+.IR DEVICE " | "
+.BI "devgroup " DEVGROUP
+.RB "} { " up " | " down " | " arp " { " on " | " off " } |"
 .br
 .BR promisc " { " on " | " off " } |"
 .br
@@ -933,6 +935,11 @@ specifies network device to operate on. When configuring SR-IOV Virtual Fuction
 device.
 
 .TP
+.BI devgroup " DEVGROUP "
+.I DEVGROUP
+specifies network device group to operate on.
+
+.TP
 .BR up " and " down
 change the state of the device to
 .B UP
-- 
1.7.1

Re: [PATCH v3 1/3] iproute2: add support for setting device groups

[Patrick McHardy]

On 26.01.2011 17:41, Vlad Dogaru wrote:
> Use the group keyword to specify what group the device should belong to.
> Since the kernel uses numbers internally, mapping of group names to
> numbers is defined in /etc/iproute2/group_map. Example usage:
> 
>   ip link set dev eth0 group default
> 
> @@ -297,6 +299,13 @@ int iplink_parse(int argc, char **argv, struct iplink_req *req,
>  			if (get_integer(&mtu, *argv, 0))
>  				invarg("Invalid \"mtu\" value\n", *argv);
>  			addattr_l(&req->n, sizeof(*req), IFLA_MTU, &mtu, 4);
> +		} else if (strcmp(*argv, "group") == 0) {
> +			NEXT_ARG();
> +			if (group != -1)
> +				duparg("group", *argv);
> +			if (lookup_map_id(*argv, &group, GROUP_MAP))
> +				invarg("Invalid \"group\" value\n", *argv);
> +			addattr_l(&req->n, sizeof(*req), IFLA_GROUP, &group, 4);

I think it would be preferrable to use a function similar to
rt_realm_n2a() that can also handle plain numerical values.

Re: [PATCH v3 1/3] iproute2: add support for setting device groups

[Vlad Dogaru]

On Wed, Feb 02, 2011 at 09:56:28AM +0100, Patrick McHardy wrote:
> On 26.01.2011 17:41, Vlad Dogaru wrote:
> > Use the group keyword to specify what group the device should belong to.
> > Since the kernel uses numbers internally, mapping of group names to
> > numbers is defined in /etc/iproute2/group_map. Example usage:
> > 
> >   ip link set dev eth0 group default
> > 
> > @@ -297,6 +299,13 @@ int iplink_parse(int argc, char **argv, struct iplink_req *req,
> >  			if (get_integer(&mtu, *argv, 0))
> >  				invarg("Invalid \"mtu\" value\n", *argv);
> >  			addattr_l(&req->n, sizeof(*req), IFLA_MTU, &mtu, 4);
> > +		} else if (strcmp(*argv, "group") == 0) {
> > +			NEXT_ARG();
> > +			if (group != -1)
> > +				duparg("group", *argv);
> > +			if (lookup_map_id(*argv, &group, GROUP_MAP))
> > +				invarg("Invalid \"group\" value\n", *argv);
> > +			addattr_l(&req->n, sizeof(*req), IFLA_GROUP, &group, 4);
> 
> I think it would be preferrable to use a function similar to
> rt_realm_n2a() that can also handle plain numerical values.

The a2n() functions are rather complex for this case: they employ
caching and store a table. I suppose this is because multiple calls to
them are possible in a single run and the correspondence has to be made
in both ways (a2n and n2a).

A network group is only converted to a number at most once for each ip
process spawned, so storing a table is not really helpful. What could,
however, help is using get_integer before lookup_map_id. Only if
get_integer fails would we lookup the symbolic group name.

Does that make sense?

Re: [PATCH v3 1/3] iproute2: add support for setting device groups

[Patrick McHardy]

On 02.02.2011 10:13, Vlad Dogaru wrote:
> On Wed, Feb 02, 2011 at 09:56:28AM +0100, Patrick McHardy wrote:
>> On 26.01.2011 17:41, Vlad Dogaru wrote:
>>> Use the group keyword to specify what group the device should belong to.
>>> Since the kernel uses numbers internally, mapping of group names to
>>> numbers is defined in /etc/iproute2/group_map. Example usage:
>>>
>>>   ip link set dev eth0 group default
>>>
>>> @@ -297,6 +299,13 @@ int iplink_parse(int argc, char **argv, struct iplink_req *req,
>>>  			if (get_integer(&mtu, *argv, 0))
>>>  				invarg("Invalid \"mtu\" value\n", *argv);
>>>  			addattr_l(&req->n, sizeof(*req), IFLA_MTU, &mtu, 4);
>>> +		} else if (strcmp(*argv, "group") == 0) {
>>> +			NEXT_ARG();
>>> +			if (group != -1)
>>> +				duparg("group", *argv);
>>> +			if (lookup_map_id(*argv, &group, GROUP_MAP))
>>> +				invarg("Invalid \"group\" value\n", *argv);
>>> +			addattr_l(&req->n, sizeof(*req), IFLA_GROUP, &group, 4);
>>
>> I think it would be preferrable to use a function similar to
>> rt_realm_n2a() that can also handle plain numerical values.
> 
> The a2n() functions are rather complex for this case: they employ
> caching and store a table. I suppose this is because multiple calls to
> them are possible in a single run and the correspondence has to be made
> in both ways (a2n and n2a).
> 
> A network group is only converted to a number at most once for each ip
> process spawned, so storing a table is not really helpful. What could,
> however, help is using get_integer before lookup_map_id. Only if
> get_integer fails would we lookup the symbolic group name.
> 
> Does that make sense?

Sure, that would be fine as well.

One more thing I find confusing is that for assigning a group
to a device the parameter is called "group", for performing
actions on a group its called "devgroup". Why not simply use
"group" for both cases? The case "ip link set devgroup X group Y"
doesn't work anyways since the IFLA_GROUP attribute is used
for both.

Re: [PATCH v3 1/3] iproute2: add support for setting device groups

[Patrick McHardy]

On 02.02.2011 10:13, Vlad Dogaru wrote:
> On Wed, Feb 02, 2011 at 09:56:28AM +0100, Patrick McHardy wrote:
>> On 26.01.2011 17:41, Vlad Dogaru wrote:
>>> Use the group keyword to specify what group the device should belong to.
>>> Since the kernel uses numbers internally, mapping of group names to
>>> numbers is defined in /etc/iproute2/group_map. Example usage:
>>>
>>>   ip link set dev eth0 group default
>>>
>>> @@ -297,6 +299,13 @@ int iplink_parse(int argc, char **argv, struct iplink_req *req,
>>>  			if (get_integer(&mtu, *argv, 0))
>>>  				invarg("Invalid \"mtu\" value\n", *argv);
>>>  			addattr_l(&req->n, sizeof(*req), IFLA_MTU, &mtu, 4);
>>> +		} else if (strcmp(*argv, "group") == 0) {
>>> +			NEXT_ARG();
>>> +			if (group != -1)
>>> +				duparg("group", *argv);
>>> +			if (lookup_map_id(*argv, &group, GROUP_MAP))
>>> +				invarg("Invalid \"group\" value\n", *argv);
>>> +			addattr_l(&req->n, sizeof(*req), IFLA_GROUP, &group, 4);
>>
>> I think it would be preferrable to use a function similar to
>> rt_realm_n2a() that can also handle plain numerical values.
> 
> The a2n() functions are rather complex for this case: they employ
> caching and store a table. I suppose this is because multiple calls to
> them are possible in a single run and the correspondence has to be made
> in both ways (a2n and n2a).
> 
> A network group is only converted to a number at most once for each ip
> process spawned, so storing a table is not really helpful. What could,
> however, help is using get_integer before lookup_map_id. Only if
> get_integer fails would we lookup the symbolic group name.

Actually that's not entirely correct, the caches are (also) maintained
to speed up batch mode, in which case there could also be multiple name
to group mappings.

Re: [PATCH v3 1/3] iproute2: add support for setting device groups

[Vlad Dogaru]

On Wed, Feb 02, 2011 at 10:23:21AM +0100, Patrick McHardy wrote:
> On 02.02.2011 10:13, Vlad Dogaru wrote:
> > On Wed, Feb 02, 2011 at 09:56:28AM +0100, Patrick McHardy wrote:
> >> On 26.01.2011 17:41, Vlad Dogaru wrote:
> >>> Use the group keyword to specify what group the device should belong to.
> >>> Since the kernel uses numbers internally, mapping of group names to
> >>> numbers is defined in /etc/iproute2/group_map. Example usage:
> >>>
> >>>   ip link set dev eth0 group default
> >>>
> >>> @@ -297,6 +299,13 @@ int iplink_parse(int argc, char **argv, struct iplink_req *req,
> >>>  			if (get_integer(&mtu, *argv, 0))
> >>>  				invarg("Invalid \"mtu\" value\n", *argv);
> >>>  			addattr_l(&req->n, sizeof(*req), IFLA_MTU, &mtu, 4);
> >>> +		} else if (strcmp(*argv, "group") == 0) {
> >>> +			NEXT_ARG();
> >>> +			if (group != -1)
> >>> +				duparg("group", *argv);
> >>> +			if (lookup_map_id(*argv, &group, GROUP_MAP))
> >>> +				invarg("Invalid \"group\" value\n", *argv);
> >>> +			addattr_l(&req->n, sizeof(*req), IFLA_GROUP, &group, 4);
> >>
> >> I think it would be preferrable to use a function similar to
> >> rt_realm_n2a() that can also handle plain numerical values.
> > 
> > The a2n() functions are rather complex for this case: they employ
> > caching and store a table. I suppose this is because multiple calls to
> > them are possible in a single run and the correspondence has to be made
> > in both ways (a2n and n2a).
> > 
> > A network group is only converted to a number at most once for each ip
> > process spawned, so storing a table is not really helpful. What could,
> > however, help is using get_integer before lookup_map_id. Only if
> > get_integer fails would we lookup the symbolic group name.
> 
> Actually that's not entirely correct, the caches are (also) maintained
> to speed up batch mode, in which case there could also be multiple name
> to group mappings.

Both comments noted. I will respin the patches dropping the devgroup
keyword and implementing caching for groups.

Thanks for the feedback.