Message ID | 1296060086-18777-2-git-send-email-ddvlad@rosedu.org |
---|---|
State | Superseded, archived |
Delegated to: | stephen hemminger |
Headers | show |
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. -- 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
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? -- 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
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. -- 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
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. -- 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
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. -- 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
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;
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