| Message ID | BC879902-DED4-4AFC-B2AA-8C4F4C4B4C99@apple.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | stephen hemminger |
| Headers | show |
| Series | [iproute2] Add "show" subcommand to "ip fou" | expand |
On Tue, 31 Oct 2017 13:00:47 -0700 Greg Greenway <ggreenway@apple.com> wrote: > + if (tb[FOU_ATTR_AF]) { > + family = rta_getattr_u8(tb[FOU_ATTR_AF]); > + if (family == AF_INET) > + family_str = "AF_INET"; > + else if (family == AF_INET6) > + family_str = "AF_INET6"; > + else > + family_str = "unknown"; > + fprintf(fp, "af %s ", family_str); The unwritten rule for ip commands is that the show function must format the output with same command syntax as the other commands set/add/delete. Since there is no "af AF_INET" option to ip fou, this breaks that convention. Either ignore the address family, change the add command, or output with same syntax (-6); preferably the latter.
On Nov 1, 2017, at 2:03 PM, Stephen Hemminger <stephen@networkplumber.org> wrote: > > On Tue, 31 Oct 2017 13:00:47 -0700 > Greg Greenway <ggreenway@apple.com> wrote: > >> + if (tb[FOU_ATTR_AF]) { >> + family = rta_getattr_u8(tb[FOU_ATTR_AF]); >> + if (family == AF_INET) >> + family_str = "AF_INET"; >> + else if (family == AF_INET6) >> + family_str = "AF_INET6"; >> + else >> + family_str = "unknown"; >> + fprintf(fp, "af %s ", family_str); > > The unwritten rule for ip commands is that the show function > must format the output with same command syntax as the other commands set/add/delete. > Since there is no "af AF_INET" option to ip fou, this breaks that convention. > Either ignore the address family, change the add command, or output with same > syntax (-6); preferably the latter. That makes sense. Here's a corrected version. It also avoids a trailing-space in the output. From: Greg Greenway <ggreenway@apple.com> Date: Tue, 31 Oct 2017 12:47:35 -0700 Subject: [PATCH] Add "show" subcommand to "ip fou". Sample output: $ sudo ./ip/ip fou add port 111 ipproto 11 $ sudo ./ip/ip fou add port 222 ipproto 22 -6 $ ./ip/ip fou show port 222 ipproto 22 -6 port 111 ipproto 11 Signed-off-by: Greg Greenway <ggreenway@apple.com> --- ip/ipfou.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 60 insertions(+) diff --git a/ip/ipfou.c b/ip/ipfou.c index 00dbe15..ecbaf11 100644 --- a/ip/ipfou.c +++ b/ip/ipfou.c @@ -28,6 +28,7 @@ static void usage(void) fprintf(stderr, "Usage: ip fou add port PORT " "{ ipproto PROTO | gue } [ -6 ]\n"); fprintf(stderr, " ip fou del port PORT [ -6 ]\n"); + fprintf(stderr, " ip fou show\n"); fprintf(stderr, "\n"); fprintf(stderr, "Where: PROTO { ipproto-name | 1..255 }\n"); fprintf(stderr, " PORT { 1..65535 }\n"); @@ -134,6 +135,63 @@ static int do_del(int argc, char **argv) return 0; } +static int print_fou_mapping(const struct sockaddr_nl *who, + struct nlmsghdr *n, void *arg) +{ + FILE *fp = (FILE *)arg; + struct genlmsghdr *ghdr; + struct rtattr *tb[FOU_ATTR_MAX + 1]; + int len = n->nlmsg_len; + unsigned family; + + if (n->nlmsg_type != genl_family) + return 0; + + len -= NLMSG_LENGTH(GENL_HDRLEN); + if (len < 0) + return -1; + + ghdr = NLMSG_DATA(n); + parse_rtattr(tb, FOU_ATTR_MAX, (void *) ghdr + GENL_HDRLEN, len); + + if (tb[FOU_ATTR_PORT]) + fprintf(fp, "port %u", ntohs(rta_getattr_u16(tb[FOU_ATTR_PORT]))); + if (tb[FOU_ATTR_TYPE] && rta_getattr_u8(tb[FOU_ATTR_TYPE]) == FOU_ENCAP_GUE) + fprintf(fp, " gue"); + else if (tb[FOU_ATTR_IPPROTO]) + fprintf(fp, " ipproto %u", rta_getattr_u8(tb[FOU_ATTR_IPPROTO])); + if (tb[FOU_ATTR_AF]) { + family = rta_getattr_u8(tb[FOU_ATTR_AF]); + if (family == AF_INET6) + fprintf(fp, " -6"); + } + fprintf(fp, "\n"); + + return 0; +} + +static int do_show(int argc, char **argv) +{ + FOU_REQUEST(req, 4096, FOU_CMD_GET, NLM_F_REQUEST | NLM_F_DUMP); + + if (argc > 0) { + fprintf(stderr, "\"ip fou show\" does not take any arguments.\n"); + return -1; + } + + if (rtnl_send(&genl_rth, &req.n, req.n.nlmsg_len) < 0) { + perror("Cannot send show request"); + exit(1); + } + + if (rtnl_dump_filter(&genl_rth, print_fou_mapping, stdout) < 0) { + fprintf(stderr, "Dump terminated\n"); + return 1; + } + + return 0; +} + int do_ipfou(int argc, char **argv) { if (argc < 1) @@ -149,6 +207,8 @@ int do_ipfou(int argc, char **argv) return do_add(argc-1, argv+1); if (matches(*argv, "delete") == 0) return do_del(argc-1, argv+1); + if (matches(*argv, "show") == 0) + return do_show(argc-1, argv+1); fprintf(stderr, "Command \"%s\" is unknown, try \"ip fou help\".\n", *argv); exit(-1); }
On Fri, Nov 3, 2017 at 10:19 AM, Greg Greenway <ggreenway@apple.com> wrote: > On Nov 1, 2017, at 2:03 PM, Stephen Hemminger <stephen@networkplumber.org> wrote: >> >> On Tue, 31 Oct 2017 13:00:47 -0700 >> Greg Greenway <ggreenway@apple.com> wrote: >> >>> + if (tb[FOU_ATTR_AF]) { >>> + family = rta_getattr_u8(tb[FOU_ATTR_AF]); >>> + if (family == AF_INET) >>> + family_str = "AF_INET"; >>> + else if (family == AF_INET6) >>> + family_str = "AF_INET6"; >>> + else >>> + family_str = "unknown"; >>> + fprintf(fp, "af %s ", family_str); >> >> The unwritten rule for ip commands is that the show function >> must format the output with same command syntax as the other commands set/add/delete. >> Since there is no "af AF_INET" option to ip fou, this breaks that convention. >> Either ignore the address family, change the add command, or output with same >> syntax (-6); preferably the latter. > > That makes sense. Here's a corrected version. It also avoids a trailing-space in the output. > > From: Greg Greenway <ggreenway@apple.com> > Date: Tue, 31 Oct 2017 12:47:35 -0700 > Subject: [PATCH] Add "show" subcommand to "ip fou". > > Sample output: > > $ sudo ./ip/ip fou add port 111 ipproto 11 > $ sudo ./ip/ip fou add port 222 ipproto 22 -6 > $ ./ip/ip fou show > port 222 ipproto 22 -6 > port 111 ipproto 11 > > Signed-off-by: Greg Greenway <ggreenway@apple.com> > --- > ip/ipfou.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 60 insertions(+) > > diff --git a/ip/ipfou.c b/ip/ipfou.c > index 00dbe15..ecbaf11 100644 > --- a/ip/ipfou.c > +++ b/ip/ipfou.c > @@ -28,6 +28,7 @@ static void usage(void) > fprintf(stderr, "Usage: ip fou add port PORT " > "{ ipproto PROTO | gue } [ -6 ]\n"); > fprintf(stderr, " ip fou del port PORT [ -6 ]\n"); > + fprintf(stderr, " ip fou show\n"); > fprintf(stderr, "\n"); > fprintf(stderr, "Where: PROTO { ipproto-name | 1..255 }\n"); > fprintf(stderr, " PORT { 1..65535 }\n"); > @@ -134,6 +135,63 @@ static int do_del(int argc, char **argv) > return 0; > } > > +static int print_fou_mapping(const struct sockaddr_nl *who, > + struct nlmsghdr *n, void *arg) > +{ > + FILE *fp = (FILE *)arg; > + struct genlmsghdr *ghdr; > + struct rtattr *tb[FOU_ATTR_MAX + 1]; > + int len = n->nlmsg_len; > + unsigned family; > + > + if (n->nlmsg_type != genl_family) > + return 0; > + > + len -= NLMSG_LENGTH(GENL_HDRLEN); > + if (len < 0) > + return -1; > + > + ghdr = NLMSG_DATA(n); > + parse_rtattr(tb, FOU_ATTR_MAX, (void *) ghdr + GENL_HDRLEN, len); > + > + if (tb[FOU_ATTR_PORT]) > + fprintf(fp, "port %u", ntohs(rta_getattr_u16(tb[FOU_ATTR_PORT]))); > + if (tb[FOU_ATTR_TYPE] && rta_getattr_u8(tb[FOU_ATTR_TYPE]) == FOU_ENCAP_GUE) > + fprintf(fp, " gue"); > + else if (tb[FOU_ATTR_IPPROTO]) > + fprintf(fp, " ipproto %u", rta_getattr_u8(tb[FOU_ATTR_IPPROTO])); > + if (tb[FOU_ATTR_AF]) { > + family = rta_getattr_u8(tb[FOU_ATTR_AF]); > + if (family == AF_INET6) > + fprintf(fp, " -6"); > + } > + fprintf(fp, "\n"); > + > + return 0; > +} > + > +static int do_show(int argc, char **argv) > +{ > + FOU_REQUEST(req, 4096, FOU_CMD_GET, NLM_F_REQUEST | NLM_F_DUMP); > + > + if (argc > 0) { > + fprintf(stderr, "\"ip fou show\" does not take any arguments.\n"); > + return -1; > + } > + > + if (rtnl_send(&genl_rth, &req.n, req.n.nlmsg_len) < 0) { > + perror("Cannot send show request"); > + exit(1); > + } > + > + if (rtnl_dump_filter(&genl_rth, print_fou_mapping, stdout) < 0) { > + fprintf(stderr, "Dump terminated\n"); > + return 1; > + } > + > + return 0; > +} > + > int do_ipfou(int argc, char **argv) > { > if (argc < 1) > @@ -149,6 +207,8 @@ int do_ipfou(int argc, char **argv) > return do_add(argc-1, argv+1); > if (matches(*argv, "delete") == 0) > return do_del(argc-1, argv+1); > + if (matches(*argv, "show") == 0) > + return do_show(argc-1, argv+1); > fprintf(stderr, "Command \"%s\" is unknown, try \"ip fou help\".\n", *argv); > exit(-1); > } > -- > 2.7.4 > Acked-by: Tom Herbert <tom@herbertland.com>
> On Nov 3, 2017, at 10:25 AM, Tom Herbert <tom@herbertland.com> wrote: > > On Fri, Nov 3, 2017 at 10:19 AM, Greg Greenway <ggreenway@apple.com> wrote: >> On Nov 1, 2017, at 2:03 PM, Stephen Hemminger <stephen@networkplumber.org> wrote: >>> >>> On Tue, 31 Oct 2017 13:00:47 -0700 >>> Greg Greenway <ggreenway@apple.com> wrote: >>> >>>> + if (tb[FOU_ATTR_AF]) { >>>> + family = rta_getattr_u8(tb[FOU_ATTR_AF]); >>>> + if (family == AF_INET) >>>> + family_str = "AF_INET"; >>>> + else if (family == AF_INET6) >>>> + family_str = "AF_INET6"; >>>> + else >>>> + family_str = "unknown"; >>>> + fprintf(fp, "af %s ", family_str); >>> >>> The unwritten rule for ip commands is that the show function >>> must format the output with same command syntax as the other commands set/add/delete. >>> Since there is no "af AF_INET" option to ip fou, this breaks that convention. >>> Either ignore the address family, change the add command, or output with same >>> syntax (-6); preferably the latter. >> >> That makes sense. Here's a corrected version. It also avoids a trailing-space in the output. >> >> From: Greg Greenway <ggreenway@apple.com> >> Date: Tue, 31 Oct 2017 12:47:35 -0700 >> Subject: [PATCH] Add "show" subcommand to "ip fou". >> >> Sample output: >> >> $ sudo ./ip/ip fou add port 111 ipproto 11 >> $ sudo ./ip/ip fou add port 222 ipproto 22 -6 >> $ ./ip/ip fou show >> port 222 ipproto 22 -6 >> port 111 ipproto 11 >> >> Signed-off-by: Greg Greenway <ggreenway@apple.com> >> --- >> ip/ipfou.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 60 insertions(+) >> >> diff --git a/ip/ipfou.c b/ip/ipfou.c >> index 00dbe15..ecbaf11 100644 >> --- a/ip/ipfou.c >> +++ b/ip/ipfou.c >> @@ -28,6 +28,7 @@ static void usage(void) >> fprintf(stderr, "Usage: ip fou add port PORT " >> "{ ipproto PROTO | gue } [ -6 ]\n"); >> fprintf(stderr, " ip fou del port PORT [ -6 ]\n"); >> + fprintf(stderr, " ip fou show\n"); >> fprintf(stderr, "\n"); >> fprintf(stderr, "Where: PROTO { ipproto-name | 1..255 }\n"); >> fprintf(stderr, " PORT { 1..65535 }\n"); >> @@ -134,6 +135,63 @@ static int do_del(int argc, char **argv) >> return 0; >> } >> >> +static int print_fou_mapping(const struct sockaddr_nl *who, >> + struct nlmsghdr *n, void *arg) >> +{ >> + FILE *fp = (FILE *)arg; >> + struct genlmsghdr *ghdr; >> + struct rtattr *tb[FOU_ATTR_MAX + 1]; >> + int len = n->nlmsg_len; >> + unsigned family; >> + >> + if (n->nlmsg_type != genl_family) >> + return 0; >> + >> + len -= NLMSG_LENGTH(GENL_HDRLEN); >> + if (len < 0) >> + return -1; >> + >> + ghdr = NLMSG_DATA(n); >> + parse_rtattr(tb, FOU_ATTR_MAX, (void *) ghdr + GENL_HDRLEN, len); >> + >> + if (tb[FOU_ATTR_PORT]) >> + fprintf(fp, "port %u", ntohs(rta_getattr_u16(tb[FOU_ATTR_PORT]))); >> + if (tb[FOU_ATTR_TYPE] && rta_getattr_u8(tb[FOU_ATTR_TYPE]) == FOU_ENCAP_GUE) >> + fprintf(fp, " gue"); >> + else if (tb[FOU_ATTR_IPPROTO]) >> + fprintf(fp, " ipproto %u", rta_getattr_u8(tb[FOU_ATTR_IPPROTO])); >> + if (tb[FOU_ATTR_AF]) { >> + family = rta_getattr_u8(tb[FOU_ATTR_AF]); >> + if (family == AF_INET6) >> + fprintf(fp, " -6"); >> + } >> + fprintf(fp, "\n"); >> + >> + return 0; >> +} >> + >> +static int do_show(int argc, char **argv) >> +{ >> + FOU_REQUEST(req, 4096, FOU_CMD_GET, NLM_F_REQUEST | NLM_F_DUMP); >> + >> + if (argc > 0) { >> + fprintf(stderr, "\"ip fou show\" does not take any arguments.\n"); >> + return -1; >> + } >> + >> + if (rtnl_send(&genl_rth, &req.n, req.n.nlmsg_len) < 0) { >> + perror("Cannot send show request"); >> + exit(1); >> + } >> + >> + if (rtnl_dump_filter(&genl_rth, print_fou_mapping, stdout) < 0) { >> + fprintf(stderr, "Dump terminated\n"); >> + return 1; >> + } >> + >> + return 0; >> +} >> + >> int do_ipfou(int argc, char **argv) >> { >> if (argc < 1) >> @@ -149,6 +207,8 @@ int do_ipfou(int argc, char **argv) >> return do_add(argc-1, argv+1); >> if (matches(*argv, "delete") == 0) >> return do_del(argc-1, argv+1); >> + if (matches(*argv, "show") == 0) >> + return do_show(argc-1, argv+1); >> fprintf(stderr, "Command \"%s\" is unknown, try \"ip fou help\".\n", *argv); >> exit(-1); >> } >> -- >> 2.7.4 >> > Acked-by: Tom Herbert <tom@herbertland.com> Are there any other issues/concerns, or anything else I need to do for this patch to be accepted? Thanks, Greg
On Fri, 03 Nov 2017 10:19:22 -0700 Greg Greenway <ggreenway@apple.com> wrote: > On Nov 1, 2017, at 2:03 PM, Stephen Hemminger <stephen@networkplumber.org> wrote: > > > > On Tue, 31 Oct 2017 13:00:47 -0700 > > Greg Greenway <ggreenway@apple.com> wrote: > > > >> + if (tb[FOU_ATTR_AF]) { > >> + family = rta_getattr_u8(tb[FOU_ATTR_AF]); > >> + if (family == AF_INET) > >> + family_str = "AF_INET"; > >> + else if (family == AF_INET6) > >> + family_str = "AF_INET6"; > >> + else > >> + family_str = "unknown"; > >> + fprintf(fp, "af %s ", family_str); > > > > The unwritten rule for ip commands is that the show function > > must format the output with same command syntax as the other commands set/add/delete. > > Since there is no "af AF_INET" option to ip fou, this breaks that convention. > > Either ignore the address family, change the add command, or output with same > > syntax (-6); preferably the latter. > > That makes sense. Here's a corrected version. It also avoids a trailing-space in the output. Yes, your followup looks correct but since it didn't follow the mailing list patch protocol it was not picked up and managed by patchwork. https://patchwork.ozlabs.org/patch/832717/ You need to post the patch as new patch (ie not a followup) with the "v2" designation in order to get it correctly picked up and managed by patchwork.
diff --git a/ip/ipfou.c b/ip/ipfou.c index 00dbe15..2eb5bfd 100644 --- a/ip/ipfou.c +++ b/ip/ipfou.c @@ -28,6 +28,7 @@ static void usage(void) fprintf(stderr, "Usage: ip fou add port PORT " "{ ipproto PROTO | gue } [ -6 ]\n"); fprintf(stderr, " ip fou del port PORT [ -6 ]\n"); + fprintf(stderr, " ip fou show\n"); fprintf(stderr, "\n"); fprintf(stderr, "Where: PROTO { ipproto-name | 1..255 }\n"); fprintf(stderr, " PORT { 1..65535 }\n"); @@ -134,6 +135,69 @@ static int do_del(int argc, char **argv) return 0; } +static int print_fou_mapping(const struct sockaddr_nl *who, + struct nlmsghdr *n, void *arg) +{ + FILE *fp = (FILE *)arg; + struct genlmsghdr *ghdr; + struct rtattr *tb[FOU_ATTR_MAX + 1]; + int len = n->nlmsg_len; + unsigned family; + char *family_str; + + if (n->nlmsg_type != genl_family) + return 0; + + len -= NLMSG_LENGTH(GENL_HDRLEN); + if (len < 0) + return -1; + + ghdr = NLMSG_DATA(n); + parse_rtattr(tb, FOU_ATTR_MAX, (void *) ghdr + GENL_HDRLEN, len); + + if (tb[FOU_ATTR_PORT]) + fprintf(fp, "port %u ", ntohs(rta_getattr_u16(tb[FOU_ATTR_PORT]))); + if (tb[FOU_ATTR_AF]) { + family = rta_getattr_u8(tb[FOU_ATTR_AF]); + if (family == AF_INET) + family_str = "AF_INET"; + else if (family == AF_INET6) + family_str = "AF_INET6"; + else + family_str = "unknown"; + fprintf(fp, "af %s ", family_str); + } + if (tb[FOU_ATTR_TYPE] && rta_getattr_u8(tb[FOU_ATTR_TYPE]) == FOU_ENCAP_GUE) + fprintf(fp, "gue "); + else if (tb[FOU_ATTR_IPPROTO]) + fprintf(fp, "ipproto %u ", rta_getattr_u8(tb[FOU_ATTR_IPPROTO])); + fprintf(fp, "\n"); + + return 0; +} + +static int do_show(int argc, char **argv) +{ + FOU_REQUEST(req, 4096, FOU_CMD_GET, NLM_F_REQUEST | NLM_F_DUMP); + + if (argc > 0) { + fprintf(stderr, "\"ip fou show\" does not take any arguments.\n"); + return -1; + } + + if (rtnl_send(&genl_rth, &req.n, req.n.nlmsg_len) < 0) { + perror("Cannot send show request"); + exit(1); + } + + if (rtnl_dump_filter(&genl_rth, print_fou_mapping, stdout) < 0) { + fprintf(stderr, "Dump terminated\n"); + return 1; + } + + return 0; +} + int do_ipfou(int argc, char **argv) { if (argc < 1) @@ -149,6 +213,8 @@ int do_ipfou(int argc, char **argv) return do_add(argc-1, argv+1); if (matches(*argv, "delete") == 0) return do_del(argc-1, argv+1); + if (matches(*argv, "show") == 0) + return do_show(argc-1, argv+1); fprintf(stderr, "Command \"%s\" is unknown, try \"ip fou help\".\n", *argv); exit(-1); }
Sample output: $ ip fou show port 4 af AF_INET ipproto 4 Signed-off-by: Greg Greenway <ggreenway@apple.com> --- ip/ipfou.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 66 insertions(+)