| Message ID | 1421391147-35021-1-git-send-email-roopa@cumulusnetworks.com |
|---|---|
| State | Superseded, archived |
| Delegated to: | stephen hemminger |
| Headers | show |
On Thu, Jan 15, 2015 at 10:52 PM, <roopa@cumulusnetworks.com> wrote: > From: Roopa Prabhu <roopa@cumulusnetworks.com> > > This patch adds vlan range support to bridge command > using the newly added vinfo flags BRIDGE_VLAN_INFO_RANGE_BEGIN and > BRIDGE_VLAN_INFO_RANGE_END. > > $bridge vlan show > port vlan ids > br0 1 PVID Egress Untagged > > dummy0 1 PVID Egress Untagged > > $bridge vlan add vid 10-15 dev dummy0 > port vlan ids > br0 1 PVID Egress Untagged > > dummy0 1 PVID Egress Untagged > 10 > 11 > 12 > 13 > 14 > 15 > > $bridge vlan del vid 14 dev dummy0 > > $bridge vlan show > port vlan ids > br0 1 PVID Egress Untagged > > dummy0 1 PVID Egress Untagged > 10 > 11 > 12 > 13 > 15 > > $bridge vlan del vid 10-15 dev dummy0 > > $bridge vlan show > port vlan ids > br0 1 PVID Egress Untagged > > dummy0 1 PVID Egress Untagged > > Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com> > Signed-off-by: Wilson Kok <wkok@cumulusnetworks.com> > --- > bridge/vlan.c | 46 ++++++++++++++++++++++++++++++++++++++------- > include/linux/if_bridge.h | 2 ++ > 2 files changed, 41 insertions(+), 7 deletions(-) > > diff --git a/bridge/vlan.c b/bridge/vlan.c > index 3bd7b0d..90b3b6b 100644 > --- a/bridge/vlan.c > +++ b/bridge/vlan.c > @@ -32,6 +32,7 @@ static int vlan_modify(int cmd, int argc, char **argv) > } req; > char *d = NULL; > short vid = -1; > + short vid_end = -1; > struct rtattr *afspec; > struct bridge_vlan_info vinfo; > unsigned short flags = 0; > @@ -49,8 +50,18 @@ static int vlan_modify(int cmd, int argc, char **argv) > NEXT_ARG(); > d = *argv; > } else if (strcmp(*argv, "vid") == 0) { > + char *p; > NEXT_ARG(); > - vid = atoi(*argv); > + p = strchr(*argv, '-'); > + if (p) { > + *p = '\0'; > + p++; > + vinfo.vid = atoi(*argv); > + vid_end = atoi(p); Is "vid 10-" same as "vid 10-0"? Is "vid -15" same as "vid 0-15"? What is "vid -"? Does the "-" char mess up shells? I don't know the answer; just asking. > + vinfo.flags |= BRIDGE_VLAN_INFO_RANGE_BEGIN; > + } else { > + vinfo.vid = atoi(*argv); > + } > } else if (strcmp(*argv, "self") == 0) { > flags |= BRIDGE_FLAGS_SELF; > } else if (strcmp(*argv, "master") == 0) { > @@ -67,7 +78,7 @@ static int vlan_modify(int cmd, int argc, char **argv) > argc--; argv++; > } > > - if (d == NULL || vid == -1) { > + if (d == NULL || vinfo.vid == -1) { Where was vinfo.vid initialized to -1? Maybe use vid rather than vinfo.vid in the code above where parsing the arg, and continue using vid and vid_end until final put of vinfo. -scott -- 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 1/17/15, 5:35 PM, Scott Feldman wrote: > On Thu, Jan 15, 2015 at 10:52 PM, <roopa@cumulusnetworks.com> wrote: >> From: Roopa Prabhu <roopa@cumulusnetworks.com> >> >> This patch adds vlan range support to bridge command >> using the newly added vinfo flags BRIDGE_VLAN_INFO_RANGE_BEGIN and >> BRIDGE_VLAN_INFO_RANGE_END. >> >> $bridge vlan show >> port vlan ids >> br0 1 PVID Egress Untagged >> >> dummy0 1 PVID Egress Untagged >> >> $bridge vlan add vid 10-15 dev dummy0 >> port vlan ids >> br0 1 PVID Egress Untagged >> >> dummy0 1 PVID Egress Untagged >> 10 >> 11 >> 12 >> 13 >> 14 >> 15 >> >> $bridge vlan del vid 14 dev dummy0 >> >> $bridge vlan show >> port vlan ids >> br0 1 PVID Egress Untagged >> >> dummy0 1 PVID Egress Untagged >> 10 >> 11 >> 12 >> 13 >> 15 >> >> $bridge vlan del vid 10-15 dev dummy0 >> >> $bridge vlan show >> port vlan ids >> br0 1 PVID Egress Untagged >> >> dummy0 1 PVID Egress Untagged >> >> Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com> >> Signed-off-by: Wilson Kok <wkok@cumulusnetworks.com> >> --- >> bridge/vlan.c | 46 ++++++++++++++++++++++++++++++++++++++------- >> include/linux/if_bridge.h | 2 ++ >> 2 files changed, 41 insertions(+), 7 deletions(-) >> >> diff --git a/bridge/vlan.c b/bridge/vlan.c >> index 3bd7b0d..90b3b6b 100644 >> --- a/bridge/vlan.c >> +++ b/bridge/vlan.c >> @@ -32,6 +32,7 @@ static int vlan_modify(int cmd, int argc, char **argv) >> } req; >> char *d = NULL; >> short vid = -1; >> + short vid_end = -1; >> struct rtattr *afspec; >> struct bridge_vlan_info vinfo; >> unsigned short flags = 0; >> @@ -49,8 +50,18 @@ static int vlan_modify(int cmd, int argc, char **argv) >> NEXT_ARG(); >> d = *argv; >> } else if (strcmp(*argv, "vid") == 0) { >> + char *p; >> NEXT_ARG(); >> - vid = atoi(*argv); >> + p = strchr(*argv, '-'); >> + if (p) { >> + *p = '\0'; >> + p++; >> + vinfo.vid = atoi(*argv); >> + vid_end = atoi(p); > Is "vid 10-" same as "vid 10-0"? correct .. # bridge vlan add vid 10- dev dummy0 Invalid VLAN range "10-0" > > Is "vid -15" same as "vid 0-15"? correct # bridge vlan add vid -10 dev dummy0 root@net-next:~/iproute2# bridge vlan show port vlan ids br0 1 PVID Egress Untagged dummy0 0 PVID 1 2 3 4 5 6 7 8 9 10 dummy1 1 PVID Egress Untagged maybe vlan 0 should not be allowed. Will check > > What is "vid -"? > > Does the "-" char mess up shells? I don't know the answer; just asking. No it does not :) # bridge vlan add vid - Device and VLAN ID are required arguments. > >> + vinfo.flags |= BRIDGE_VLAN_INFO_RANGE_BEGIN; >> + } else { >> + vinfo.vid = atoi(*argv); >> + } >> } else if (strcmp(*argv, "self") == 0) { >> flags |= BRIDGE_FLAGS_SELF; >> } else if (strcmp(*argv, "master") == 0) { >> @@ -67,7 +78,7 @@ static int vlan_modify(int cmd, int argc, char **argv) >> argc--; argv++; >> } >> >> - if (d == NULL || vid == -1) { >> + if (d == NULL || vinfo.vid == -1) { > Where was vinfo.vid initialized to -1? Maybe use vid rather than > vinfo.vid in the code above where parsing the arg, and continue using > vid and vid_end until final put of vinfo. > There is already a "memset(&vinfo, 0, sizeof(vinfo));" in the code in the beginning of the function. And the code is already using vinfo for flags. That's the reason i decided to use vinfo here. Thanks, Roopa -- 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 Sun, Jan 18, 2015 at 1:11 AM, roopa <roopa@cumulusnetworks.com> wrote: > On 1/17/15, 5:35 PM, Scott Feldman wrote: >> >> On Thu, Jan 15, 2015 at 10:52 PM, <roopa@cumulusnetworks.com> wrote: >>> >>> From: Roopa Prabhu <roopa@cumulusnetworks.com> >>> >>> This patch adds vlan range support to bridge command >>> using the newly added vinfo flags BRIDGE_VLAN_INFO_RANGE_BEGIN and >>> BRIDGE_VLAN_INFO_RANGE_END. >>> > >> >>> + vinfo.flags |= >>> BRIDGE_VLAN_INFO_RANGE_BEGIN; >>> + } else { >>> + vinfo.vid = atoi(*argv); >>> + } >>> } else if (strcmp(*argv, "self") == 0) { >>> flags |= BRIDGE_FLAGS_SELF; >>> } else if (strcmp(*argv, "master") == 0) { >>> @@ -67,7 +78,7 @@ static int vlan_modify(int cmd, int argc, char **argv) >>> argc--; argv++; >>> } >>> >>> - if (d == NULL || vid == -1) { >>> + if (d == NULL || vinfo.vid == -1) { >> >> Where was vinfo.vid initialized to -1? Maybe use vid rather than >> vinfo.vid in the code above where parsing the arg, and continue using >> vid and vid_end until final put of vinfo. >> > There is already a "memset(&vinfo, 0, sizeof(vinfo));" in the code in the > beginning of the function. That's the problem...vinfo.vid is initialized to 0, not -1, so checking if vinfo.vid == -1 is always false. -- 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 1/18/15, 9:44 AM, Scott Feldman wrote: > On Sun, Jan 18, 2015 at 1:11 AM, roopa <roopa@cumulusnetworks.com> wrote: >> On 1/17/15, 5:35 PM, Scott Feldman wrote: >>> On Thu, Jan 15, 2015 at 10:52 PM, <roopa@cumulusnetworks.com> wrote: >>>> From: Roopa Prabhu <roopa@cumulusnetworks.com> >>>> >>>> This patch adds vlan range support to bridge command >>>> using the newly added vinfo flags BRIDGE_VLAN_INFO_RANGE_BEGIN and >>>> BRIDGE_VLAN_INFO_RANGE_END. >>>> >>>> + vinfo.flags |= >>>> BRIDGE_VLAN_INFO_RANGE_BEGIN; >>>> + } else { >>>> + vinfo.vid = atoi(*argv); >>>> + } >>>> } else if (strcmp(*argv, "self") == 0) { >>>> flags |= BRIDGE_FLAGS_SELF; >>>> } else if (strcmp(*argv, "master") == 0) { >>>> @@ -67,7 +78,7 @@ static int vlan_modify(int cmd, int argc, char **argv) >>>> argc--; argv++; >>>> } >>>> >>>> - if (d == NULL || vid == -1) { >>>> + if (d == NULL || vinfo.vid == -1) { >>> Where was vinfo.vid initialized to -1? Maybe use vid rather than >>> vinfo.vid in the code above where parsing the arg, and continue using >>> vid and vid_end until final put of vinfo. >>> >> There is already a "memset(&vinfo, 0, sizeof(vinfo));" in the code in the >> beginning of the function. > That's the problem...vinfo.vid is initialized to 0, not -1, so > checking if vinfo.vid == -1 is always false. ack, v2 coming ... thanks -- 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/bridge/vlan.c b/bridge/vlan.c index 3bd7b0d..90b3b6b 100644 --- a/bridge/vlan.c +++ b/bridge/vlan.c @@ -32,6 +32,7 @@ static int vlan_modify(int cmd, int argc, char **argv) } req; char *d = NULL; short vid = -1; + short vid_end = -1; struct rtattr *afspec; struct bridge_vlan_info vinfo; unsigned short flags = 0; @@ -49,8 +50,18 @@ static int vlan_modify(int cmd, int argc, char **argv) NEXT_ARG(); d = *argv; } else if (strcmp(*argv, "vid") == 0) { + char *p; NEXT_ARG(); - vid = atoi(*argv); + p = strchr(*argv, '-'); + if (p) { + *p = '\0'; + p++; + vinfo.vid = atoi(*argv); + vid_end = atoi(p); + vinfo.flags |= BRIDGE_VLAN_INFO_RANGE_BEGIN; + } else { + vinfo.vid = atoi(*argv); + } } else if (strcmp(*argv, "self") == 0) { flags |= BRIDGE_FLAGS_SELF; } else if (strcmp(*argv, "master") == 0) { @@ -67,7 +78,7 @@ static int vlan_modify(int cmd, int argc, char **argv) argc--; argv++; } - if (d == NULL || vid == -1) { + if (d == NULL || vinfo.vid == -1) { fprintf(stderr, "Device and VLAN ID are required arguments.\n"); exit(-1); } @@ -78,20 +89,41 @@ static int vlan_modify(int cmd, int argc, char **argv) return -1; } - if (vid >= 4096) { - fprintf(stderr, "Invalid VLAN ID \"%hu\"\n", vid); + if (vinfo.vid >= 4096) { + fprintf(stderr, "Invalid VLAN ID \"%hu\"\n", vinfo.vid); return -1; } - vinfo.vid = vid; + if (vinfo.flags & BRIDGE_VLAN_INFO_RANGE_BEGIN) { + if (vid_end == -1 || vid_end >= 4096 || vinfo.vid >= vid_end) { + fprintf(stderr, "Invalid VLAN range \"%hu-%hu\"\n", + vinfo.vid, vid_end); + return -1; + } + if (vinfo.flags & BRIDGE_VLAN_INFO_PVID) { + fprintf(stderr, + "pvid cannot be configured for a vlan range\n"); + return -1; + } + } afspec = addattr_nest(&req.n, sizeof(req), IFLA_AF_SPEC); if (flags) addattr16(&req.n, sizeof(req), IFLA_BRIDGE_FLAGS, flags); - addattr_l(&req.n, sizeof(req), IFLA_BRIDGE_VLAN_INFO, &vinfo, - sizeof(vinfo)); + if (vid_end != -1) { + addattr_l(&req.n, sizeof(req), IFLA_BRIDGE_VLAN_INFO, &vinfo, + sizeof(vinfo)); + vinfo.flags &= ~BRIDGE_VLAN_INFO_RANGE_BEGIN; + vinfo.flags |= BRIDGE_VLAN_INFO_RANGE_END; + vinfo.vid = vid_end; + addattr_l(&req.n, sizeof(req), IFLA_BRIDGE_VLAN_INFO, &vinfo, + sizeof(vinfo)); + } else { + addattr_l(&req.n, sizeof(req), IFLA_BRIDGE_VLAN_INFO, &vinfo, + sizeof(vinfo)); + } addattr_nest_end(&req.n, afspec); diff --git a/include/linux/if_bridge.h b/include/linux/if_bridge.h index ed6868e..e21a649 100644 --- a/include/linux/if_bridge.h +++ b/include/linux/if_bridge.h @@ -124,6 +124,8 @@ enum { #define BRIDGE_VLAN_INFO_MASTER (1<<0) /* Operate on Bridge device as well */ #define BRIDGE_VLAN_INFO_PVID (1<<1) /* VLAN is PVID, ingress untagged */ #define BRIDGE_VLAN_INFO_UNTAGGED (1<<2) /* VLAN egresses untagged */ +#define BRIDGE_VLAN_INFO_RANGE_BEGIN (1<<3) /* VLAN is start of vlan range */ +#define BRIDGE_VLAN_INFO_RANGE_END (1<<4) /* VLAN is end of vlan range */ struct bridge_vlan_info { __u16 flags;