Re: ospfctl json support
Hi, Any chance someone could take a look at this, we have written a prometheus exporter for bgp and ospf, which we would like to share, but it relies on having this json support in place. I have re-attached the original diff. Thanks Richard On Mon, Jun 1, 2020 at 7:52 AM Richard Chivers wrote: > Hi, > > Just a quick bump, wondering if someone could take a look at the diff, > conscious there appears to have been a lot going on recently! > > I want to do some more detailed work on the json output, but would > ideally get this first iteration in first. > > Richard > > On Sat, May 23, 2020 at 9:28 AM Richard Chivers > wrote: > > > > Hi, > > > > I have attached the first iteration of the ospf json support. Sorry > > about the large commit, > > but it had to come in one go, if we didn't want a broken implementation. > > > > I will go back and update output.c and output_json.c to remove the > detail flag > > and instead pass through the cli parse_result as is done in bgpctl -j sh > rib, > > but thought it made sense to get the main commit out of the way and > > then circle back > > to these tweaks next week. > > > > In general I think there should be a consideration over how times are > output. > > > > At present this diff outputs time using the pretty standard approach > > used in ospfctl all along, > > which is also what bgpctl -j sh rib does. > > > > I question whether for durations and uptime we should perhaps > > introduce something like > > > > uptime_seconds: 30 > > uptime_seconds: null > > > > as well as or rather than: > > > > uptime: "6d06h31m" > > uptime: "00:00:00" > > > > I'm not sure if the above is a standard format (6d06h31m) that can be > parsed > > easily in many languages, which is part of what json support adds to the > mix, > > if it is then it is the best of both worlds already! > > > > I can take a dig into this, but i'm sure someone will know off hand, > > if so what is the RFC or equiv? > > > > The only aspects I have not tested are: > > > > ospfctl -j show database summary > > ospfctl -j show database opaque > > > > These have no results in my configuration. > > > > I do question wheter ospfctl -j show database summary just needs > removing? > > I will perhaps have a dig into ospfd and see what generates the relevant > imsg. > ospfctl_json.diff Description: Binary data
Re: ospfctl json support
Hi, Just a quick bump, wondering if someone could take a look at the diff, conscious there appears to have been a lot going on recently! I want to do some more detailed work on the json output, but would ideally get this first iteration in first. Richard On Sat, May 23, 2020 at 9:28 AM Richard Chivers wrote: > > Hi, > > I have attached the first iteration of the ospf json support. Sorry > about the large commit, > but it had to come in one go, if we didn't want a broken implementation. > > I will go back and update output.c and output_json.c to remove the detail flag > and instead pass through the cli parse_result as is done in bgpctl -j sh rib, > but thought it made sense to get the main commit out of the way and > then circle back > to these tweaks next week. > > In general I think there should be a consideration over how times are output. > > At present this diff outputs time using the pretty standard approach > used in ospfctl all along, > which is also what bgpctl -j sh rib does. > > I question whether for durations and uptime we should perhaps > introduce something like > > uptime_seconds: 30 > uptime_seconds: null > > as well as or rather than: > > uptime: "6d06h31m" > uptime: "00:00:00" > > I'm not sure if the above is a standard format (6d06h31m) that can be parsed > easily in many languages, which is part of what json support adds to the mix, > if it is then it is the best of both worlds already! > > I can take a dig into this, but i'm sure someone will know off hand, > if so what is the RFC or equiv? > > The only aspects I have not tested are: > > ospfctl -j show database summary > ospfctl -j show database opaque > > These have no results in my configuration. > > I do question wheter ospfctl -j show database summary just needs removing? > I will perhaps have a dig into ospfd and see what generates the relevant imsg.
ospfctl json support
Hi, I have attached the first iteration of the ospf json support. Sorry about the large commit, but it had to come in one go, if we didn't want a broken implementation. I will go back and update output.c and output_json.c to remove the detail flag and instead pass through the cli parse_result as is done in bgpctl -j sh rib, but thought it made sense to get the main commit out of the way and then circle back to these tweaks next week. In general I think there should be a consideration over how times are output. At present this diff outputs time using the pretty standard approach used in ospfctl all along, which is also what bgpctl -j sh rib does. I question whether for durations and uptime we should perhaps introduce something like uptime_seconds: 30 uptime_seconds: null as well as or rather than: uptime: "6d06h31m" uptime: "00:00:00" I'm not sure if the above is a standard format (6d06h31m) that can be parsed easily in many languages, which is part of what json support adds to the mix, if it is then it is the best of both worlds already! I can take a dig into this, but i'm sure someone will know off hand, if so what is the RFC or equiv? The only aspects I have not tested are: ospfctl -j show database summary ospfctl -j show database opaque These have no results in my configuration. I do question wheter ospfctl -j show database summary just needs removing? I will perhaps have a dig into ospfd and see what generates the relevant imsg. ospfctl_json.diff Description: Binary data
Re: ospfctl json support
On Mon, May 18, 2020 at 02:46:52PM +0100, Richard Chivers wrote:
> Hi,
>
> Thanks for the feedback, I wasn't happy with the detail flag either, I
> just missed coming back to rethink / change it.
>
> There was a lot of duplicate code i seem to remember, as much of the
> output is sent regardless.
>
> I will take a look over the next couple of days when I get a few
> minutes to retest etc.
>
> I will also check the style(9) I imagine I have the wrong line length
> configured
Most issues I have seen are missing spaces around if() and }else{.
> In terms of process, shall I do that first before anything else happens?
> Sorry not familiar with the review process etc.
Do it in that order you think works best. Keep the diffs small to help to
move them quickly.
> On Mon, May 18, 2020 at 11:00 AM Claudio Jeker
> wrote:
> >
> > On Mon, May 18, 2020 at 09:49:24AM +0200, Denis Fondras wrote:
> > > On Mon, May 18, 2020 at 09:04:06AM +0200, Claudio Jeker wrote:
> > > > There is a file missing in the diff.
> > > >
> > > > One thing I have seen in the original diff from Richard was that the
> > > > copyright in the new file should be copied from ospfctl.c since this is
> > > > mostly a copy paste action and not new work.
> > > >
> > >
> > > Stupid me... Here is an update.
> > > Thank you Claudio.
> >
> > Not sure about the detail switches. Like this one:
> >
> > > + case IMSG_CTL_SHOW_INTERFACE:
> > > + ctliface = imsg->data;
> > > + if(res->action == SHOW_IFACE_DTAIL)
> > > + output->interface(ctliface, 1);
> > > + else
> > > + output->interface(ctliface, 0);
> > > + break;
> >
> > I pushed those down into the show functions so that the show() function in
> > bgpctl does not do any kind of decision on the output.
> >
> > I looked over the code. There is more KNF fixes needed and maybe some
> > additional changes (like return a response object for all calls) that can
> > happen in tree.
> >
> > OK claudio@
> >
> > > Index: Makefile
> > > ===
> > > RCS file: /cvs/src/usr.sbin/ospfctl/Makefile,v
> > > retrieving revision 1.5
> > > diff -u -p -r1.5 Makefile
> > > --- Makefile 2 Sep 2016 14:02:48 - 1.5
> > > +++ Makefile 17 May 2020 10:51:28 -
> > > @@ -3,7 +3,7 @@
> > > .PATH: ${.CURDIR}/../ospfd
> > >
> > > PROG=ospfctl
> > > -SRCS=logmsg.c ospfctl.c parser.c
> > > +SRCS=logmsg.c ospfctl.c output.c parser.c
> > > CFLAGS+= -Wall
> > > CFLAGS+= -Wstrict-prototypes -Wmissing-prototypes
> > > CFLAGS+= -Wshadow -Wpointer-arith -Wcast-qual
> > > Index: ospfctl.c
> > > ===
> > > RCS file: /cvs/src/usr.sbin/ospfctl/ospfctl.c,v
> > > retrieving revision 1.66
> > > diff -u -p -r1.66 ospfctl.c
> > > --- ospfctl.c 1 Nov 2019 18:15:28 - 1.66
> > > +++ ospfctl.c 17 May 2020 11:11:50 -
> > > @@ -35,42 +35,16 @@
> > >
> > > #include "ospf.h"
> > > #include "ospfd.h"
> > > +#include "ospfctl.h"
> > > #include "ospfe.h"
> > > #include "parser.h"
> > >
> > > __dead void usage(void);
> > > -int show_summary_msg(struct imsg *);
> > > -uint64_t get_ifms_type(uint8_t);
> > > -int show_interface_msg(struct imsg *);
> > > -int show_interface_detail_msg(struct imsg *);
> > > -const char *print_link(int);
> > > -const char *fmt_timeframe(time_t t);
> > > -const char *fmt_timeframe_core(time_t t);
> > > -const char *log_id(u_int32_t );
> > > -const char *log_adv_rtr(u_int32_t);
> > > -void show_database_head(struct in_addr, char *, u_int8_t);
> > > -int show_database_msg(struct imsg *);
> > > -char *print_ls_type(u_int8_t);
> > > -void show_db_hdr_msg_detail(struct lsa_hdr *);
> > > -char *print_rtr_link_type(u_int8_t);
> > > -const char *print_ospf_flags(u_int8_t);
> > > -int show_db_msg_detail(struct imsg *imsg);
> > > -int show_nbr_msg(struct imsg *);
> > > -const char *print_ospf_options(u_int8_t);
> > > -int show_nbr_detail_msg(struct imsg *);
> > > -int show_rib_msg(struct imsg *);
> > > -void show_rib_head(struct in_addr, u_int8_t, u_int8_t);
> > > -const char *print_ospf_rtr_flags(u_int8_t);
> > > -int show_rib_detail_msg(struct imsg *);
> > > -void show_fib_head(void);
> > > -int show_fib_msg(struct imsg *);
> > > -void show_interface_head(void);
> > > -const char * get_media_descr(uint64_t);
> > > -const char * get_linkstate(uint8_t, int);
> > > -void print_baudrate(u_int64_t);
> > > -int show_fib_interface_msg(struct imsg *);
> > > +
> > > +int show(struct imsg *, struct parse_result *);
> > >
> > > struct imsgbuf *ibuf;
> > > +const struct output *output = &show_output;
> > >
> > > __dead void
> > > usage(void)
> > > @@ -
Re: ospfctl json support
Hi,
Thanks for the feedback, I wasn't happy with the detail flag either, I
just missed coming back to rethink / change it.
There was a lot of duplicate code i seem to remember, as much of the
output is sent regardless.
I will take a look over the next couple of days when I get a few
minutes to retest etc.
I will also check the style(9) I imagine I have the wrong line length
configured
In terms of process, shall I do that first before anything else happens?
Sorry not familiar with the review process etc.
On Mon, May 18, 2020 at 11:00 AM Claudio Jeker wrote:
>
> On Mon, May 18, 2020 at 09:49:24AM +0200, Denis Fondras wrote:
> > On Mon, May 18, 2020 at 09:04:06AM +0200, Claudio Jeker wrote:
> > > There is a file missing in the diff.
> > >
> > > One thing I have seen in the original diff from Richard was that the
> > > copyright in the new file should be copied from ospfctl.c since this is
> > > mostly a copy paste action and not new work.
> > >
> >
> > Stupid me... Here is an update.
> > Thank you Claudio.
>
> Not sure about the detail switches. Like this one:
>
> > + case IMSG_CTL_SHOW_INTERFACE:
> > + ctliface = imsg->data;
> > + if(res->action == SHOW_IFACE_DTAIL)
> > + output->interface(ctliface, 1);
> > + else
> > + output->interface(ctliface, 0);
> > + break;
>
> I pushed those down into the show functions so that the show() function in
> bgpctl does not do any kind of decision on the output.
>
> I looked over the code. There is more KNF fixes needed and maybe some
> additional changes (like return a response object for all calls) that can
> happen in tree.
>
> OK claudio@
>
> > Index: Makefile
> > ===
> > RCS file: /cvs/src/usr.sbin/ospfctl/Makefile,v
> > retrieving revision 1.5
> > diff -u -p -r1.5 Makefile
> > --- Makefile 2 Sep 2016 14:02:48 - 1.5
> > +++ Makefile 17 May 2020 10:51:28 -
> > @@ -3,7 +3,7 @@
> > .PATH: ${.CURDIR}/../ospfd
> >
> > PROG=ospfctl
> > -SRCS=logmsg.c ospfctl.c parser.c
> > +SRCS=logmsg.c ospfctl.c output.c parser.c
> > CFLAGS+= -Wall
> > CFLAGS+= -Wstrict-prototypes -Wmissing-prototypes
> > CFLAGS+= -Wshadow -Wpointer-arith -Wcast-qual
> > Index: ospfctl.c
> > ===
> > RCS file: /cvs/src/usr.sbin/ospfctl/ospfctl.c,v
> > retrieving revision 1.66
> > diff -u -p -r1.66 ospfctl.c
> > --- ospfctl.c 1 Nov 2019 18:15:28 - 1.66
> > +++ ospfctl.c 17 May 2020 11:11:50 -
> > @@ -35,42 +35,16 @@
> >
> > #include "ospf.h"
> > #include "ospfd.h"
> > +#include "ospfctl.h"
> > #include "ospfe.h"
> > #include "parser.h"
> >
> > __dead void usage(void);
> > -int show_summary_msg(struct imsg *);
> > -uint64_t get_ifms_type(uint8_t);
> > -int show_interface_msg(struct imsg *);
> > -int show_interface_detail_msg(struct imsg *);
> > -const char *print_link(int);
> > -const char *fmt_timeframe(time_t t);
> > -const char *fmt_timeframe_core(time_t t);
> > -const char *log_id(u_int32_t );
> > -const char *log_adv_rtr(u_int32_t);
> > -void show_database_head(struct in_addr, char *, u_int8_t);
> > -int show_database_msg(struct imsg *);
> > -char *print_ls_type(u_int8_t);
> > -void show_db_hdr_msg_detail(struct lsa_hdr *);
> > -char *print_rtr_link_type(u_int8_t);
> > -const char *print_ospf_flags(u_int8_t);
> > -int show_db_msg_detail(struct imsg *imsg);
> > -int show_nbr_msg(struct imsg *);
> > -const char *print_ospf_options(u_int8_t);
> > -int show_nbr_detail_msg(struct imsg *);
> > -int show_rib_msg(struct imsg *);
> > -void show_rib_head(struct in_addr, u_int8_t, u_int8_t);
> > -const char *print_ospf_rtr_flags(u_int8_t);
> > -int show_rib_detail_msg(struct imsg *);
> > -void show_fib_head(void);
> > -int show_fib_msg(struct imsg *);
> > -void show_interface_head(void);
> > -const char * get_media_descr(uint64_t);
> > -const char * get_linkstate(uint8_t, int);
> > -void print_baudrate(u_int64_t);
> > -int show_fib_interface_msg(struct imsg *);
> > +
> > +int show(struct imsg *, struct parse_result *);
> >
> > struct imsgbuf *ibuf;
> > +const struct output *output = &show_output;
> >
> > __dead void
> > usage(void)
> > @@ -145,10 +119,6 @@ main(int argc, char *argv[])
> > imsg_compose(ibuf, IMSG_CTL_SHOW_SUM, 0, 0, -1, NULL, 0);
> > break;
> > case SHOW_IFACE:
> > - printf("%-11s %-18s %-6s %-10s %-10s %-8s %3s %3s\n",
> > - "Interface", "Address", "State", "HelloTimer",
> > "Linkstate",
> > - "Uptime", "nc", "ac");
> > - /*FALLTHROUGH*/
> > case SHOW_IFACE_DTAIL:
> >
Re: ospfctl json support
On Mon, May 18, 2020 at 09:49:24AM +0200, Denis Fondras wrote:
> On Mon, May 18, 2020 at 09:04:06AM +0200, Claudio Jeker wrote:
> > There is a file missing in the diff.
> >
> > One thing I have seen in the original diff from Richard was that the
> > copyright in the new file should be copied from ospfctl.c since this is
> > mostly a copy paste action and not new work.
> >
>
> Stupid me... Here is an update.
> Thank you Claudio.
Not sure about the detail switches. Like this one:
> + case IMSG_CTL_SHOW_INTERFACE:
> + ctliface = imsg->data;
> + if(res->action == SHOW_IFACE_DTAIL)
> + output->interface(ctliface, 1);
> + else
> + output->interface(ctliface, 0);
> + break;
I pushed those down into the show functions so that the show() function in
bgpctl does not do any kind of decision on the output.
I looked over the code. There is more KNF fixes needed and maybe some
additional changes (like return a response object for all calls) that can
happen in tree.
OK claudio@
> Index: Makefile
> ===
> RCS file: /cvs/src/usr.sbin/ospfctl/Makefile,v
> retrieving revision 1.5
> diff -u -p -r1.5 Makefile
> --- Makefile 2 Sep 2016 14:02:48 - 1.5
> +++ Makefile 17 May 2020 10:51:28 -
> @@ -3,7 +3,7 @@
> .PATH: ${.CURDIR}/../ospfd
>
> PROG=ospfctl
> -SRCS=logmsg.c ospfctl.c parser.c
> +SRCS=logmsg.c ospfctl.c output.c parser.c
> CFLAGS+= -Wall
> CFLAGS+= -Wstrict-prototypes -Wmissing-prototypes
> CFLAGS+= -Wshadow -Wpointer-arith -Wcast-qual
> Index: ospfctl.c
> ===
> RCS file: /cvs/src/usr.sbin/ospfctl/ospfctl.c,v
> retrieving revision 1.66
> diff -u -p -r1.66 ospfctl.c
> --- ospfctl.c 1 Nov 2019 18:15:28 - 1.66
> +++ ospfctl.c 17 May 2020 11:11:50 -
> @@ -35,42 +35,16 @@
>
> #include "ospf.h"
> #include "ospfd.h"
> +#include "ospfctl.h"
> #include "ospfe.h"
> #include "parser.h"
>
> __dead void usage(void);
> -int show_summary_msg(struct imsg *);
> -uint64_t get_ifms_type(uint8_t);
> -int show_interface_msg(struct imsg *);
> -int show_interface_detail_msg(struct imsg *);
> -const char *print_link(int);
> -const char *fmt_timeframe(time_t t);
> -const char *fmt_timeframe_core(time_t t);
> -const char *log_id(u_int32_t );
> -const char *log_adv_rtr(u_int32_t);
> -void show_database_head(struct in_addr, char *, u_int8_t);
> -int show_database_msg(struct imsg *);
> -char *print_ls_type(u_int8_t);
> -void show_db_hdr_msg_detail(struct lsa_hdr *);
> -char *print_rtr_link_type(u_int8_t);
> -const char *print_ospf_flags(u_int8_t);
> -int show_db_msg_detail(struct imsg *imsg);
> -int show_nbr_msg(struct imsg *);
> -const char *print_ospf_options(u_int8_t);
> -int show_nbr_detail_msg(struct imsg *);
> -int show_rib_msg(struct imsg *);
> -void show_rib_head(struct in_addr, u_int8_t, u_int8_t);
> -const char *print_ospf_rtr_flags(u_int8_t);
> -int show_rib_detail_msg(struct imsg *);
> -void show_fib_head(void);
> -int show_fib_msg(struct imsg *);
> -void show_interface_head(void);
> -const char * get_media_descr(uint64_t);
> -const char * get_linkstate(uint8_t, int);
> -void print_baudrate(u_int64_t);
> -int show_fib_interface_msg(struct imsg *);
> +
> +int show(struct imsg *, struct parse_result *);
>
> struct imsgbuf *ibuf;
> +const struct output *output = &show_output;
>
> __dead void
> usage(void)
> @@ -145,10 +119,6 @@ main(int argc, char *argv[])
> imsg_compose(ibuf, IMSG_CTL_SHOW_SUM, 0, 0, -1, NULL, 0);
> break;
> case SHOW_IFACE:
> - printf("%-11s %-18s %-6s %-10s %-10s %-8s %3s %3s\n",
> - "Interface", "Address", "State", "HelloTimer", "Linkstate",
> - "Uptime", "nc", "ac");
> - /*FALLTHROUGH*/
> case SHOW_IFACE_DTAIL:
> if (*res->ifname) {
> ifidx = if_nametoindex(res->ifname);
> @@ -159,9 +129,6 @@ main(int argc, char *argv[])
> &ifidx, sizeof(ifidx));
> break;
> case SHOW_NBR:
> - printf("%-15s %-3s %-12s %-8s %-15s %-9s %s\n", "ID", "Pri",
> - "State", "DeadTime", "Address", "Iface","Uptime");
> - /*FALLTHROUGH*/
> case SHOW_NBR_DTAIL:
> imsg_compose(ibuf, IMSG_CTL_SHOW_NBR, 0, 0, -1, NULL, 0);
> break;
> @@ -194,9 +161,6 @@ main(int argc, char *argv[])
> imsg_compose(ibuf, IMSG_CTL_SHOW_DB_OPAQ, 0, 0, -1, NULL, 0);
> break;
> case SHOW_RIB:
> - printf("%-20s %-17s %-12s %-9s %-7s %-8s\n", "Destination",
> -
Re: ospfctl json support
On Mon, May 18, 2020 at 09:04:06AM +0200, Claudio Jeker wrote:
> There is a file missing in the diff.
>
> One thing I have seen in the original diff from Richard was that the
> copyright in the new file should be copied from ospfctl.c since this is
> mostly a copy paste action and not new work.
>
Stupid me... Here is an update.
Thank you Claudio.
Index: Makefile
===
RCS file: /cvs/src/usr.sbin/ospfctl/Makefile,v
retrieving revision 1.5
diff -u -p -r1.5 Makefile
--- Makefile2 Sep 2016 14:02:48 - 1.5
+++ Makefile17 May 2020 10:51:28 -
@@ -3,7 +3,7 @@
.PATH: ${.CURDIR}/../ospfd
PROG= ospfctl
-SRCS= logmsg.c ospfctl.c parser.c
+SRCS= logmsg.c ospfctl.c output.c parser.c
CFLAGS+= -Wall
CFLAGS+= -Wstrict-prototypes -Wmissing-prototypes
CFLAGS+= -Wshadow -Wpointer-arith -Wcast-qual
Index: ospfctl.c
===
RCS file: /cvs/src/usr.sbin/ospfctl/ospfctl.c,v
retrieving revision 1.66
diff -u -p -r1.66 ospfctl.c
--- ospfctl.c 1 Nov 2019 18:15:28 - 1.66
+++ ospfctl.c 17 May 2020 11:11:50 -
@@ -35,42 +35,16 @@
#include "ospf.h"
#include "ospfd.h"
+#include "ospfctl.h"
#include "ospfe.h"
#include "parser.h"
__dead void usage(void);
-int show_summary_msg(struct imsg *);
-uint64_tget_ifms_type(uint8_t);
-int show_interface_msg(struct imsg *);
-int show_interface_detail_msg(struct imsg *);
-const char *print_link(int);
-const char *fmt_timeframe(time_t t);
-const char *fmt_timeframe_core(time_t t);
-const char *log_id(u_int32_t );
-const char *log_adv_rtr(u_int32_t);
-voidshow_database_head(struct in_addr, char *, u_int8_t);
-int show_database_msg(struct imsg *);
-char *print_ls_type(u_int8_t);
-voidshow_db_hdr_msg_detail(struct lsa_hdr *);
-char *print_rtr_link_type(u_int8_t);
-const char *print_ospf_flags(u_int8_t);
-int show_db_msg_detail(struct imsg *imsg);
-int show_nbr_msg(struct imsg *);
-const char *print_ospf_options(u_int8_t);
-int show_nbr_detail_msg(struct imsg *);
-int show_rib_msg(struct imsg *);
-voidshow_rib_head(struct in_addr, u_int8_t, u_int8_t);
-const char *print_ospf_rtr_flags(u_int8_t);
-int show_rib_detail_msg(struct imsg *);
-voidshow_fib_head(void);
-int show_fib_msg(struct imsg *);
-voidshow_interface_head(void);
-const char *get_media_descr(uint64_t);
-const char *get_linkstate(uint8_t, int);
-voidprint_baudrate(u_int64_t);
-int show_fib_interface_msg(struct imsg *);
+
+int show(struct imsg *, struct parse_result *);
struct imsgbuf *ibuf;
+const struct output*output = &show_output;
__dead void
usage(void)
@@ -145,10 +119,6 @@ main(int argc, char *argv[])
imsg_compose(ibuf, IMSG_CTL_SHOW_SUM, 0, 0, -1, NULL, 0);
break;
case SHOW_IFACE:
- printf("%-11s %-18s %-6s %-10s %-10s %-8s %3s %3s\n",
- "Interface", "Address", "State", "HelloTimer", "Linkstate",
- "Uptime", "nc", "ac");
- /*FALLTHROUGH*/
case SHOW_IFACE_DTAIL:
if (*res->ifname) {
ifidx = if_nametoindex(res->ifname);
@@ -159,9 +129,6 @@ main(int argc, char *argv[])
&ifidx, sizeof(ifidx));
break;
case SHOW_NBR:
- printf("%-15s %-3s %-12s %-8s %-15s %-9s %s\n", "ID", "Pri",
- "State", "DeadTime", "Address", "Iface","Uptime");
- /*FALLTHROUGH*/
case SHOW_NBR_DTAIL:
imsg_compose(ibuf, IMSG_CTL_SHOW_NBR, 0, 0, -1, NULL, 0);
break;
@@ -194,9 +161,6 @@ main(int argc, char *argv[])
imsg_compose(ibuf, IMSG_CTL_SHOW_DB_OPAQ, 0, 0, -1, NULL, 0);
break;
case SHOW_RIB:
- printf("%-20s %-17s %-12s %-9s %-7s %-8s\n", "Destination",
- "Nexthop", "Path Type", "Type", "Cost", "Uptime");
- /*FALLTHROUGH*/
case SHOW_RIB_DTAIL:
imsg_compose(ibuf, IMSG_CTL_SHOW_RIB, 0, 0, -1, NULL, 0);
break;
@@ -207,7 +171,6 @@ main(int argc, char *argv[])
else
imsg_compose(ibuf, IMSG_CTL_KROUTE_ADDR, 0, 0, -1,
&res->addr, sizeof(res->addr));
- show_fib_head();
break;
case SHOW_FIB_IFACE:
if (*res->ifname)
@@ -215,7 +178,6 @@ main(int argc, char *argv[])
res->ifname, sizeof(res->ifname));
else
imsg_compose(ibuf, IMSG_CTL_IFINFO, 0, 0, -1, NULL, 0);
- show_interface_head();
break;
case
Re: ospfctl json support
On Sun, May 17, 2020 at 04:32:33PM +0200, Denis Fondras wrote:
> On Fri, May 15, 2020 at 11:34:58AM +0100, Richard Chivers wrote:
> > Hi,
> >
> > I have now resolved the spacing/tabbing issues I think correctly
> > following style(9), along with a couple of other indent issues.
> >
> > Would appreciate a cursory look at this stage to spot any further common
> > issues.
> >
>
> I fixed some indent and break long lines.
>
> It reads OK for me. A quick test shows it works for basic commands.
>
> OK denis@
>
> Anyone else for a OK ?
There is a file missing in the diff.
One thing I have seen in the original diff from Richard was that the
copyright in the new file should be copied from ospfctl.c since this is
mostly a copy paste action and not new work.
> Index: Makefile
> ===
> RCS file: /cvs/src/usr.sbin/ospfctl/Makefile,v
> retrieving revision 1.5
> diff -u -p -r1.5 Makefile
> --- Makefile 2 Sep 2016 14:02:48 - 1.5
> +++ Makefile 17 May 2020 10:51:28 -
> @@ -3,7 +3,7 @@
> .PATH: ${.CURDIR}/../ospfd
>
> PROG=ospfctl
> -SRCS=logmsg.c ospfctl.c parser.c
> +SRCS=logmsg.c ospfctl.c output.c parser.c
> CFLAGS+= -Wall
> CFLAGS+= -Wstrict-prototypes -Wmissing-prototypes
> CFLAGS+= -Wshadow -Wpointer-arith -Wcast-qual
> Index: ospfctl.c
> ===
> RCS file: /cvs/src/usr.sbin/ospfctl/ospfctl.c,v
> retrieving revision 1.66
> diff -u -p -r1.66 ospfctl.c
> --- ospfctl.c 1 Nov 2019 18:15:28 - 1.66
> +++ ospfctl.c 17 May 2020 11:11:50 -
> @@ -35,42 +35,16 @@
>
> #include "ospf.h"
> #include "ospfd.h"
> +#include "ospfctl.h"
> #include "ospfe.h"
> #include "parser.h"
>
> __dead void usage(void);
> -int show_summary_msg(struct imsg *);
> -uint64_t get_ifms_type(uint8_t);
> -int show_interface_msg(struct imsg *);
> -int show_interface_detail_msg(struct imsg *);
> -const char *print_link(int);
> -const char *fmt_timeframe(time_t t);
> -const char *fmt_timeframe_core(time_t t);
> -const char *log_id(u_int32_t );
> -const char *log_adv_rtr(u_int32_t);
> -void show_database_head(struct in_addr, char *, u_int8_t);
> -int show_database_msg(struct imsg *);
> -char *print_ls_type(u_int8_t);
> -void show_db_hdr_msg_detail(struct lsa_hdr *);
> -char *print_rtr_link_type(u_int8_t);
> -const char *print_ospf_flags(u_int8_t);
> -int show_db_msg_detail(struct imsg *imsg);
> -int show_nbr_msg(struct imsg *);
> -const char *print_ospf_options(u_int8_t);
> -int show_nbr_detail_msg(struct imsg *);
> -int show_rib_msg(struct imsg *);
> -void show_rib_head(struct in_addr, u_int8_t, u_int8_t);
> -const char *print_ospf_rtr_flags(u_int8_t);
> -int show_rib_detail_msg(struct imsg *);
> -void show_fib_head(void);
> -int show_fib_msg(struct imsg *);
> -void show_interface_head(void);
> -const char * get_media_descr(uint64_t);
> -const char * get_linkstate(uint8_t, int);
> -void print_baudrate(u_int64_t);
> -int show_fib_interface_msg(struct imsg *);
> +
> +int show(struct imsg *, struct parse_result *);
>
> struct imsgbuf *ibuf;
> +const struct output *output = &show_output;
>
> __dead void
> usage(void)
> @@ -145,10 +119,6 @@ main(int argc, char *argv[])
> imsg_compose(ibuf, IMSG_CTL_SHOW_SUM, 0, 0, -1, NULL, 0);
> break;
> case SHOW_IFACE:
> - printf("%-11s %-18s %-6s %-10s %-10s %-8s %3s %3s\n",
> - "Interface", "Address", "State", "HelloTimer", "Linkstate",
> - "Uptime", "nc", "ac");
> - /*FALLTHROUGH*/
> case SHOW_IFACE_DTAIL:
> if (*res->ifname) {
> ifidx = if_nametoindex(res->ifname);
> @@ -159,9 +129,6 @@ main(int argc, char *argv[])
> &ifidx, sizeof(ifidx));
> break;
> case SHOW_NBR:
> - printf("%-15s %-3s %-12s %-8s %-15s %-9s %s\n", "ID", "Pri",
> - "State", "DeadTime", "Address", "Iface","Uptime");
> - /*FALLTHROUGH*/
> case SHOW_NBR_DTAIL:
> imsg_compose(ibuf, IMSG_CTL_SHOW_NBR, 0, 0, -1, NULL, 0);
> break;
> @@ -194,9 +161,6 @@ main(int argc, char *argv[])
> imsg_compose(ibuf, IMSG_CTL_SHOW_DB_OPAQ, 0, 0, -1, NULL, 0);
> break;
> case SHOW_RIB:
> - printf("%-20s %-17s %-12s %-9s %-7s %-8s\n", "Destination",
> - "Nexthop", "Path Type", "Type", "Cost", "Uptime");
> - /*FALLTHROUGH*/
> case SHOW_RIB_DTAIL:
> imsg_compose(ibuf, IMSG_CTL_SHOW_RIB, 0, 0, -1, NULL, 0);
> break;
> @@ -207,7 +171,6 @@ main(int argc, char *argv[])
> else
>
Re: ospfctl json support
On Fri, May 15, 2020 at 11:34:58AM +0100, Richard Chivers wrote:
> Hi,
>
> I have now resolved the spacing/tabbing issues I think correctly
> following style(9), along with a couple of other indent issues.
>
> Would appreciate a cursory look at this stage to spot any further common
> issues.
>
I fixed some indent and break long lines.
It reads OK for me. A quick test shows it works for basic commands.
OK denis@
Anyone else for a OK ?
Index: Makefile
===
RCS file: /cvs/src/usr.sbin/ospfctl/Makefile,v
retrieving revision 1.5
diff -u -p -r1.5 Makefile
--- Makefile2 Sep 2016 14:02:48 - 1.5
+++ Makefile17 May 2020 10:51:28 -
@@ -3,7 +3,7 @@
.PATH: ${.CURDIR}/../ospfd
PROG= ospfctl
-SRCS= logmsg.c ospfctl.c parser.c
+SRCS= logmsg.c ospfctl.c output.c parser.c
CFLAGS+= -Wall
CFLAGS+= -Wstrict-prototypes -Wmissing-prototypes
CFLAGS+= -Wshadow -Wpointer-arith -Wcast-qual
Index: ospfctl.c
===
RCS file: /cvs/src/usr.sbin/ospfctl/ospfctl.c,v
retrieving revision 1.66
diff -u -p -r1.66 ospfctl.c
--- ospfctl.c 1 Nov 2019 18:15:28 - 1.66
+++ ospfctl.c 17 May 2020 11:11:50 -
@@ -35,42 +35,16 @@
#include "ospf.h"
#include "ospfd.h"
+#include "ospfctl.h"
#include "ospfe.h"
#include "parser.h"
__dead void usage(void);
-int show_summary_msg(struct imsg *);
-uint64_tget_ifms_type(uint8_t);
-int show_interface_msg(struct imsg *);
-int show_interface_detail_msg(struct imsg *);
-const char *print_link(int);
-const char *fmt_timeframe(time_t t);
-const char *fmt_timeframe_core(time_t t);
-const char *log_id(u_int32_t );
-const char *log_adv_rtr(u_int32_t);
-voidshow_database_head(struct in_addr, char *, u_int8_t);
-int show_database_msg(struct imsg *);
-char *print_ls_type(u_int8_t);
-voidshow_db_hdr_msg_detail(struct lsa_hdr *);
-char *print_rtr_link_type(u_int8_t);
-const char *print_ospf_flags(u_int8_t);
-int show_db_msg_detail(struct imsg *imsg);
-int show_nbr_msg(struct imsg *);
-const char *print_ospf_options(u_int8_t);
-int show_nbr_detail_msg(struct imsg *);
-int show_rib_msg(struct imsg *);
-voidshow_rib_head(struct in_addr, u_int8_t, u_int8_t);
-const char *print_ospf_rtr_flags(u_int8_t);
-int show_rib_detail_msg(struct imsg *);
-voidshow_fib_head(void);
-int show_fib_msg(struct imsg *);
-voidshow_interface_head(void);
-const char *get_media_descr(uint64_t);
-const char *get_linkstate(uint8_t, int);
-voidprint_baudrate(u_int64_t);
-int show_fib_interface_msg(struct imsg *);
+
+int show(struct imsg *, struct parse_result *);
struct imsgbuf *ibuf;
+const struct output*output = &show_output;
__dead void
usage(void)
@@ -145,10 +119,6 @@ main(int argc, char *argv[])
imsg_compose(ibuf, IMSG_CTL_SHOW_SUM, 0, 0, -1, NULL, 0);
break;
case SHOW_IFACE:
- printf("%-11s %-18s %-6s %-10s %-10s %-8s %3s %3s\n",
- "Interface", "Address", "State", "HelloTimer", "Linkstate",
- "Uptime", "nc", "ac");
- /*FALLTHROUGH*/
case SHOW_IFACE_DTAIL:
if (*res->ifname) {
ifidx = if_nametoindex(res->ifname);
@@ -159,9 +129,6 @@ main(int argc, char *argv[])
&ifidx, sizeof(ifidx));
break;
case SHOW_NBR:
- printf("%-15s %-3s %-12s %-8s %-15s %-9s %s\n", "ID", "Pri",
- "State", "DeadTime", "Address", "Iface","Uptime");
- /*FALLTHROUGH*/
case SHOW_NBR_DTAIL:
imsg_compose(ibuf, IMSG_CTL_SHOW_NBR, 0, 0, -1, NULL, 0);
break;
@@ -194,9 +161,6 @@ main(int argc, char *argv[])
imsg_compose(ibuf, IMSG_CTL_SHOW_DB_OPAQ, 0, 0, -1, NULL, 0);
break;
case SHOW_RIB:
- printf("%-20s %-17s %-12s %-9s %-7s %-8s\n", "Destination",
- "Nexthop", "Path Type", "Type", "Cost", "Uptime");
- /*FALLTHROUGH*/
case SHOW_RIB_DTAIL:
imsg_compose(ibuf, IMSG_CTL_SHOW_RIB, 0, 0, -1, NULL, 0);
break;
@@ -207,7 +171,6 @@ main(int argc, char *argv[])
else
imsg_compose(ibuf, IMSG_CTL_KROUTE_ADDR, 0, 0, -1,
&res->addr, sizeof(res->addr));
- show_fib_head();
break;
case SHOW_FIB_IFACE:
if (*res->ifname)
@@ -215,7 +178,6 @@ main(int argc, char *argv[])
res->ifname, sizeof(res->ifname));
else
imsg_compose(ibuf, IMSG_CTL_IFINFO, 0
Re: ospfctl json support
Hi, I have now resolved the spacing/tabbing issues I think correctly following style(9), along with a couple of other indent issues. Would appreciate a cursory look at this stage to spot any further common issues. I have read through style(9) and can't spot anything else, but I may be missing something >From a workflow perspective can anyone suggest good tools that work with style(9) rules? I have spotted indent.1, is this generally in peoples workflow or is it typically a manual approach to each file? Cheers Richard On Thu, May 14, 2020 at 7:33 PM Denis Fondras wrote: > > On Thu, May 14, 2020 at 07:15:41PM +0100, Richard Chivers wrote: > > Shall I effectively fix issues in the original code at this stage, or only > > where I have moved and refactored? > > > > Thanks. Limit the changes to what is relative to json support. The diff is > already big enough :) ospfctl.diff Description: Binary data
Re: ospfctl json support
On Thu, May 14, 2020 at 07:15:41PM +0100, Richard Chivers wrote: > Shall I effectively fix issues in the original code at this stage, or only > where I have moved and refactored? > Thanks. Limit the changes to what is relative to json support. The diff is already big enough :)
Re: ospfctl json support
Hi, Sure I will check it out. Much of the code came directly from ospfctl, so spaces were maintained for example, I wasn't sure if I should change all lines effectively changing the original code. The code originally didn't seem to fully match what I initially read in style(9). I will have a good look through and come back over the next couple of days. Shall I effectively fix issues in the original code at this stage, or only where I have moved and refactored? Cheers Rich On Thu, 14 May 2020, 18:47 Denis Fondras, wrote: > On Thu, May 14, 2020 at 05:51:58PM +0100, Richard Chivers wrote: > > Let me know if this now works for you. > > > > This is better, I can apply it :) > However, there are many style(9) issues. Can you fix them please before I > review > the changes ? > > Thank you. >
Re: ospfctl json support
On Thu, May 14, 2020 at 05:51:58PM +0100, Richard Chivers wrote: > Let me know if this now works for you. > This is better, I can apply it :) However, there are many style(9) issues. Can you fix them please before I review the changes ? Thank you.
Re: ospfctl json support
Hi,
Sorry I thought I had done it correctly, I sent it to a colleague
first to apply, but we were
using the git mirror, just because we use git a lot generally.
I have now set-up cvs and run:
cvs diff -Nu usr.sbin/ospfctl/output.c usr.sbin/ospfctl/ospfctl.c
usr.sbin/ospfctl/ospfctl.h usr.sbin/ospfctl/Makefile > ospfctl.diff
I have also attached the diff to the email, as others appear to have
done that in the past.
If there are still issues I will set-up another cvs repo and apply the
patch to it, to test what
is going wrong, sorry not so familiar with cvs.
Let me know if this now works for you.
Thanks
Rich
On Thu, May 14, 2020 at 8:03 AM Denis Fondras wrote:
>
> Please provide a properly formatted diff.
>
> On Thu, May 14, 2020 at 07:16:31AM +0100, Richard Chivers wrote:
> > Hi,
> >
> > I have done the work to implement ospfctl json support, but as
> > discussed i will provide it in two diffs.
> >
> > This first one externalises the output aspect of ospfctl and there are
> > some things like tail that are not needed specifically for straight
> > standard output, but are required for json support.
> >
> > I also wasn't sure what to do with Copyright messages at the top of
> > files, any advice appreciated.
> >
> > In terms of outstanding issues, not sure how big to make the array in
> > print_baudrate, I guessed 32 would cover things?
> >
> > Many of the functions that return output fragments are called print,
> > when they actually return strings. in bgpctl many of these seem to
> > have been renamed to fmt_. I have left these as is for now to again
> > reduce the size of the change.
> >
> >
> > diff --git a/usr.sbin/ospfctl/Makefile b/usr.sbin/ospfctl/Makefile
> > index cfd5e4ccb71..6560e0d5f89 100644
> > --- a/usr.sbin/ospfctl/Makefile
> > +++ b/usr.sbin/ospfctl/Makefile
> > @@ -3,7 +3,7 @@
> > .PATH: ${.CURDIR}/../ospfd
> > PROG= ospfctl
> > -SRCS= logmsg.c ospfctl.c parser.c
> > +SRCS= logmsg.c ospfctl.c output.c parser.c
> > CFLAGS+= -Wall
> > CFLAGS+= -Wstrict-prototypes -Wmissing-prototypes
> > CFLAGS+= -Wshadow -Wpointer-arith -Wcast-qual
> > diff --git a/usr.sbin/ospfctl/ospfctl.c b/usr.sbin/ospfctl/ospfctl.c
> > index 2d7189793d8..e04124853a1 100644
> > --- a/usr.sbin/ospfctl/ospfctl.c
> > +++ b/usr.sbin/ospfctl/ospfctl.c
> > @@ -35,42 +35,16 @@
> > #include "ospf.h"
> > #include "ospfd.h"
> > +#include "ospfctl.h"
> > #include "ospfe.h"
> > #include "parser.h"
> > __dead void usage(void);
> > -int show_summary_msg(struct imsg *);
> > -uint64_t get_ifms_type(uint8_t);
> > -int show_interface_msg(struct imsg *);
> > -int show_interface_detail_msg(struct imsg *);
> > -const char *print_link(int);
> > -const char *fmt_timeframe(time_t t);
> > -const char *fmt_timeframe_core(time_t t);
> > -const char *log_id(u_int32_t );
> > -const char *log_adv_rtr(u_int32_t);
> > -void show_database_head(struct in_addr, char *, u_int8_t);
> > -int show_database_msg(struct imsg *);
> > -char *print_ls_type(u_int8_t);
> > -void show_db_hdr_msg_detail(struct lsa_hdr *);
> > -char *print_rtr_link_type(u_int8_t);
> > -const char *print_ospf_flags(u_int8_t);
> > -int show_db_msg_detail(struct imsg *imsg);
> > -int show_nbr_msg(struct imsg *);
> > -const char *print_ospf_options(u_int8_t);
> > -int show_nbr_detail_msg(struct imsg *);
> > -int show_rib_msg(struct imsg *);
> > -void show_rib_head(struct in_addr, u_int8_t, u_int8_t);
> > -const char *print_ospf_rtr_flags(u_int8_t);
> > -int show_rib_detail_msg(struct imsg *);
> > -void show_fib_head(void);
> > -int show_fib_msg(struct imsg *);
> > -void show_interface_head(void);
> > -const char * get_media_descr(uint64_t);
> > -const char * get_linkstate(uint8_t, int);
> > -void print_baudrate(u_int64_t);
> > -int show_fib_interface_msg(struct imsg *);
> > +
> > +int show(struct imsg *imsg, struct parse_result *res);
> > struct imsgbuf *ibuf;
> > +const struct output *output = &show_output;
> > __dead void
> > usage(void)
> > @@ -145,9 +119,6 @@ main(int argc, char *argv[])
> > imsg_compose(ibuf, IMSG_CTL_SHOW_SUM, 0, 0, -1, NULL, 0);
> > break;
> > case SHOW_IFACE:
> > - printf("%-11s %-18s %-6s %-10s %-10s %-8s %3s %3s\n",
> > - "Interface", "Address", "State", "HelloTimer", "Linkstate",
> > - "Uptime", "nc", "ac");
> > /*FALLTHROUGH*/
> > case SHOW_IFACE_DTAIL:
> &
Re: ospfctl json support
Please provide a properly formatted diff.
On Thu, May 14, 2020 at 07:16:31AM +0100, Richard Chivers wrote:
> Hi,
>
> I have done the work to implement ospfctl json support, but as
> discussed i will provide it in two diffs.
>
> This first one externalises the output aspect of ospfctl and there are
> some things like tail that are not needed specifically for straight
> standard output, but are required for json support.
>
> I also wasn't sure what to do with Copyright messages at the top of
> files, any advice appreciated.
>
> In terms of outstanding issues, not sure how big to make the array in
> print_baudrate, I guessed 32 would cover things?
>
> Many of the functions that return output fragments are called print,
> when they actually return strings. in bgpctl many of these seem to
> have been renamed to fmt_. I have left these as is for now to again
> reduce the size of the change.
>
>
> diff --git a/usr.sbin/ospfctl/Makefile b/usr.sbin/ospfctl/Makefile
> index cfd5e4ccb71..6560e0d5f89 100644
> --- a/usr.sbin/ospfctl/Makefile
> +++ b/usr.sbin/ospfctl/Makefile
> @@ -3,7 +3,7 @@
> .PATH: ${.CURDIR}/../ospfd
> PROG= ospfctl
> -SRCS= logmsg.c ospfctl.c parser.c
> +SRCS= logmsg.c ospfctl.c output.c parser.c
> CFLAGS+= -Wall
> CFLAGS+= -Wstrict-prototypes -Wmissing-prototypes
> CFLAGS+= -Wshadow -Wpointer-arith -Wcast-qual
> diff --git a/usr.sbin/ospfctl/ospfctl.c b/usr.sbin/ospfctl/ospfctl.c
> index 2d7189793d8..e04124853a1 100644
> --- a/usr.sbin/ospfctl/ospfctl.c
> +++ b/usr.sbin/ospfctl/ospfctl.c
> @@ -35,42 +35,16 @@
> #include "ospf.h"
> #include "ospfd.h"
> +#include "ospfctl.h"
> #include "ospfe.h"
> #include "parser.h"
> __dead void usage(void);
> -int show_summary_msg(struct imsg *);
> -uint64_t get_ifms_type(uint8_t);
> -int show_interface_msg(struct imsg *);
> -int show_interface_detail_msg(struct imsg *);
> -const char *print_link(int);
> -const char *fmt_timeframe(time_t t);
> -const char *fmt_timeframe_core(time_t t);
> -const char *log_id(u_int32_t );
> -const char *log_adv_rtr(u_int32_t);
> -void show_database_head(struct in_addr, char *, u_int8_t);
> -int show_database_msg(struct imsg *);
> -char *print_ls_type(u_int8_t);
> -void show_db_hdr_msg_detail(struct lsa_hdr *);
> -char *print_rtr_link_type(u_int8_t);
> -const char *print_ospf_flags(u_int8_t);
> -int show_db_msg_detail(struct imsg *imsg);
> -int show_nbr_msg(struct imsg *);
> -const char *print_ospf_options(u_int8_t);
> -int show_nbr_detail_msg(struct imsg *);
> -int show_rib_msg(struct imsg *);
> -void show_rib_head(struct in_addr, u_int8_t, u_int8_t);
> -const char *print_ospf_rtr_flags(u_int8_t);
> -int show_rib_detail_msg(struct imsg *);
> -void show_fib_head(void);
> -int show_fib_msg(struct imsg *);
> -void show_interface_head(void);
> -const char * get_media_descr(uint64_t);
> -const char * get_linkstate(uint8_t, int);
> -void print_baudrate(u_int64_t);
> -int show_fib_interface_msg(struct imsg *);
> +
> +int show(struct imsg *imsg, struct parse_result *res);
> struct imsgbuf *ibuf;
> +const struct output *output = &show_output;
> __dead void
> usage(void)
> @@ -145,9 +119,6 @@ main(int argc, char *argv[])
> imsg_compose(ibuf, IMSG_CTL_SHOW_SUM, 0, 0, -1, NULL, 0);
> break;
> case SHOW_IFACE:
> - printf("%-11s %-18s %-6s %-10s %-10s %-8s %3s %3s\n",
> - "Interface", "Address", "State", "HelloTimer", "Linkstate",
> - "Uptime", "nc", "ac");
> /*FALLTHROUGH*/
> case SHOW_IFACE_DTAIL:
> if (*res->ifname) {
> @@ -159,8 +130,6 @@ main(int argc, char *argv[])
> &ifidx, sizeof(ifidx));
> break;
> case SHOW_NBR:
> - printf("%-15s %-3s %-12s %-8s %-15s %-9s %s\n", "ID", "Pri",
> - "State", "DeadTime", "Address", "Iface","Uptime");
> /*FALLTHROUGH*/
> case SHOW_NBR_DTAIL:
> imsg_compose(ibuf, IMSG_CTL_SHOW_NBR, 0, 0, -1, NULL, 0);
> @@ -194,8 +163,6 @@ main(int argc, char *argv[])
> imsg_compose(ibuf, IMSG_CTL_SHOW_DB_OPAQ, 0, 0, -1, NULL, 0);
> break;
> case SHOW_RIB:
> - printf("%-20s %-17s %-12s %-9s %-7s %-8s\n", "Destination",
> - "Nexthop", "Path Type", "Type", "Cost", "Uptime");
> /*FALLTHROUGH*/
> case SHOW_RIB_DTAIL:
> imsg_compose(ibuf, IMSG_CTL_SHOW_RIB, 0, 0, -1, NULL, 0);
> @@ -207,7 +174,6 @@ main(int argc, char *argv[])
> else
> imsg_compose(ibuf, IMSG_CTL_KROUTE_ADDR, 0, 0, -1,
> &res->addr, sizeof(res->addr));
> - show_fib_head();
> break;
> case SHOW_FIB_IFACE:
Re: ospfctl json support
Hi,
I have done the work to implement ospfctl json support, but as
discussed i will provide it in two diffs.
This first one externalises the output aspect of ospfctl and there are
some things like tail that are not needed specifically for straight
standard output, but are required for json support.
I also wasn't sure what to do with Copyright messages at the top of
files, any advice appreciated.
In terms of outstanding issues, not sure how big to make the array in
print_baudrate, I guessed 32 would cover things?
Many of the functions that return output fragments are called print,
when they actually return strings. in bgpctl many of these seem to
have been renamed to fmt_. I have left these as is for now to again
reduce the size of the change.
diff --git a/usr.sbin/ospfctl/Makefile b/usr.sbin/ospfctl/Makefile
index cfd5e4ccb71..6560e0d5f89 100644
--- a/usr.sbin/ospfctl/Makefile
+++ b/usr.sbin/ospfctl/Makefile
@@ -3,7 +3,7 @@
.PATH: ${.CURDIR}/../ospfd
PROG= ospfctl
-SRCS= logmsg.c ospfctl.c parser.c
+SRCS= logmsg.c ospfctl.c output.c parser.c
CFLAGS+= -Wall
CFLAGS+= -Wstrict-prototypes -Wmissing-prototypes
CFLAGS+= -Wshadow -Wpointer-arith -Wcast-qual
diff --git a/usr.sbin/ospfctl/ospfctl.c b/usr.sbin/ospfctl/ospfctl.c
index 2d7189793d8..e04124853a1 100644
--- a/usr.sbin/ospfctl/ospfctl.c
+++ b/usr.sbin/ospfctl/ospfctl.c
@@ -35,42 +35,16 @@
#include "ospf.h"
#include "ospfd.h"
+#include "ospfctl.h"
#include "ospfe.h"
#include "parser.h"
__dead void usage(void);
-int show_summary_msg(struct imsg *);
-uint64_t get_ifms_type(uint8_t);
-int show_interface_msg(struct imsg *);
-int show_interface_detail_msg(struct imsg *);
-const char *print_link(int);
-const char *fmt_timeframe(time_t t);
-const char *fmt_timeframe_core(time_t t);
-const char *log_id(u_int32_t );
-const char *log_adv_rtr(u_int32_t);
-void show_database_head(struct in_addr, char *, u_int8_t);
-int show_database_msg(struct imsg *);
-char *print_ls_type(u_int8_t);
-void show_db_hdr_msg_detail(struct lsa_hdr *);
-char *print_rtr_link_type(u_int8_t);
-const char *print_ospf_flags(u_int8_t);
-int show_db_msg_detail(struct imsg *imsg);
-int show_nbr_msg(struct imsg *);
-const char *print_ospf_options(u_int8_t);
-int show_nbr_detail_msg(struct imsg *);
-int show_rib_msg(struct imsg *);
-void show_rib_head(struct in_addr, u_int8_t, u_int8_t);
-const char *print_ospf_rtr_flags(u_int8_t);
-int show_rib_detail_msg(struct imsg *);
-void show_fib_head(void);
-int show_fib_msg(struct imsg *);
-void show_interface_head(void);
-const char * get_media_descr(uint64_t);
-const char * get_linkstate(uint8_t, int);
-void print_baudrate(u_int64_t);
-int show_fib_interface_msg(struct imsg *);
+
+int show(struct imsg *imsg, struct parse_result *res);
struct imsgbuf *ibuf;
+const struct output *output = &show_output;
__dead void
usage(void)
@@ -145,9 +119,6 @@ main(int argc, char *argv[])
imsg_compose(ibuf, IMSG_CTL_SHOW_SUM, 0, 0, -1, NULL, 0);
break;
case SHOW_IFACE:
- printf("%-11s %-18s %-6s %-10s %-10s %-8s %3s %3s\n",
- "Interface", "Address", "State", "HelloTimer", "Linkstate",
- "Uptime", "nc", "ac");
/*FALLTHROUGH*/
case SHOW_IFACE_DTAIL:
if (*res->ifname) {
@@ -159,8 +130,6 @@ main(int argc, char *argv[])
&ifidx, sizeof(ifidx));
break;
case SHOW_NBR:
- printf("%-15s %-3s %-12s %-8s %-15s %-9s %s\n", "ID", "Pri",
- "State", "DeadTime", "Address", "Iface","Uptime");
/*FALLTHROUGH*/
case SHOW_NBR_DTAIL:
imsg_compose(ibuf, IMSG_CTL_SHOW_NBR, 0, 0, -1, NULL, 0);
@@ -194,8 +163,6 @@ main(int argc, char *argv[])
imsg_compose(ibuf, IMSG_CTL_SHOW_DB_OPAQ, 0, 0, -1, NULL, 0);
break;
case SHOW_RIB:
- printf("%-20s %-17s %-12s %-9s %-7s %-8s\n", "Destination",
- "Nexthop", "Path Type", "Type", "Cost", "Uptime");
/*FALLTHROUGH*/
case SHOW_RIB_DTAIL:
imsg_compose(ibuf, IMSG_CTL_SHOW_RIB, 0, 0, -1, NULL, 0);
@@ -207,7 +174,6 @@ main(int argc, char *argv[])
else
imsg_compose(ibuf, IMSG_CTL_KROUTE_ADDR, 0, 0, -1,
&res->addr, sizeof(res->addr));
- show_fib_head();
break;
case SHOW_FIB_IFACE:
if (*res->ifname)
@@ -215,7 +181,6 @@ main(int argc, char *argv[])
res->ifname, sizeof(res->ifname));
else
imsg_compose(ibuf, IMSG_CTL_IFINFO, 0, 0, -1, NULL, 0);
- show_interface_head();
break;
case FIB:
errx(1, "fib couple|decouple");
@@ -255,72 +220,30 @@ main(int argc, char *argv[])
if (msgbuf_write(&ibuf->w) <= 0 && errno != EAGAIN)
err(1, "write error");
- while (!done) {
- if ((n = imsg_read(ibuf)) == -1 && errno != EAGAIN)
- errx(1, "imsg_read error");
- if (n == 0)
- errx(1, "pipe closed");
+ // Don't attempt output for certain commands such as log verbose
+ if(!done){
+ output-
Re: ospfctl json support
Hi,
Behaviour for certain actions needs discussion and the behaviour crosses over
with the work on bgpctl. A command such as bgpctl -j log brief currently
results in a return such as:
“
Logging request sent.
{
}
"
I think this behaviour would be common to anything which effectively does a
fire and forget. We could either remove the json output by doing something
like (but not!):
if(!done) {
//output head
//output tail
}
This would stop a json response altogether. Alternatively we could update
the code to return something like:
“{ response: “Logging request sent” }
Finally we could just error if you call one of those methods with a -j flag?
Just hit the same pattern/issue on ospfctl, so looking to source some
direction. Using openbsd, there has always felt like lots of synergy
between cli interfaces, so just hoping to help continue that :)
Cheers
Richard
> On 11 May 2020, at 13:52, Claudio Jeker wrote:
>
> On Mon, May 11, 2020 at 12:38:38PM +0100, Richard Chivers wrote:
>> Hi,
>>
>> I have done some work over the last few days to implement json support
>> into ospfctl following the work done recently in bgpctl.
>>
>> I have some queries, hoping to get some help with.
>>
>> The change involves a refactor of ospfctl, but reuses the recent
>> json.c written by Claudio, that is in the
>> usr.sbin/bgpctl directory. At present no changes have been required at all.
>> What is the best approach here, should/could this be centralised somewhere?
>
> At the moment just copy the files into ospfctl. We did the same thing with
> other bits in the tree. My json API is super minimal and is for sure not
> something that should be put into a common framework right now. The way
> objects and arrays are opened and closed is a bit rough and does not
> always work well.
>
>> In some cases there is room for change, for example ospfctl sh rib &
>> ospfctl sh rib detail.
>>
>> In my view here, it makes sense to have a full list returned rather
>> than splitting json into multiple lists of Router, Network and
>> External.
>> I looked for inspiration in the bgpctl, but couldn't find a similar
>> pattern. The reason for a single list is that if consuming the json,
>> I expect you will not want code that has to iterate over three
>> separate arrays. Just looking for some feedback really. The original
>> split
>> made sense for screen use, just not so sure about machine readability.
>> This issue also applies to ospfctl sh data, which returns 3 lists.
>
> A lot of this is depending on the imsgs used between ospfctl and ospfd.
> IIRC ospfd sends all data in one batch so the multiple lists just happen
> by ospfctl and indeed for json output it would be best to display the rib
> as a single array of entries (which have a similar structure but probably
> per LSA specific attributes).
>
>> When I am finished, should I just post the diff on here? Just
>> conscious it is quite a big refactor, albeit much of the code is
>> reused just moved into output.c as
>> was done for bgpctl.
>
> Please split it up like I did it for bgpctl. I first did a lot of the
> moving and cleanup and only later added the new input mode. This makes the
> individual steps smaller to review.
>
>> I am testing each call individually against a set of openbsd 6.6 boxes
>> we have running, which is great for ospfd. What is the normal
>> practise for ospf6, is there a script run to replicate code changes or
>> is it just a case of making very similar changes line by line?
>
> There is no script, you do it by hand it is quicker.
>
>> Just looking for the general thinking and approach I guess. I don't
>> currently have ospf6 set-up so that would be some overhead.
>> Happy to configure it though if needed/expected.
>
> Get ospfctl first, after that ospf6ctl can be done (and maybe somebody
> else will take care of that).
>
>> Finally what was the driver under bgpctl for the json output, ours is
>> for reading metrics to populate telemetry, just interested as the
>> purpose other
>> people have. Having this context will help in micro decisions when
>> implementing the json structure.
>
> I'm doing this work to provide JSON payloads to external looking glasses.
> I would not put the RIB into telemetry (that is just useless churn and
> load on the telemetry system). In bgpctl the terse outputs are simple
> space separated outputs for telemetry scripting but I guess people will
> also use the JSON output for this.
>
>> As a final thought is this something that is actually wanted
>> generally, I assumed it was as bgpctl has gone/is going in that
>> direction, but just don't want to assume.
>
> I see no reason why not. At least I wont veto it.
>
> --
> :wq Claudio
Re: ospfctl json support
On Mon, May 11, 2020 at 12:38:38PM +0100, Richard Chivers wrote: > Hi, > > I have done some work over the last few days to implement json support > into ospfctl following the work done recently in bgpctl. > > I have some queries, hoping to get some help with. > > The change involves a refactor of ospfctl, but reuses the recent > json.c written by Claudio, that is in the > usr.sbin/bgpctl directory. At present no changes have been required at all. > What is the best approach here, should/could this be centralised somewhere? At the moment just copy the files into ospfctl. We did the same thing with other bits in the tree. My json API is super minimal and is for sure not something that should be put into a common framework right now. The way objects and arrays are opened and closed is a bit rough and does not always work well. > In some cases there is room for change, for example ospfctl sh rib & > ospfctl sh rib detail. > > In my view here, it makes sense to have a full list returned rather > than splitting json into multiple lists of Router, Network and > External. > I looked for inspiration in the bgpctl, but couldn't find a similar > pattern. The reason for a single list is that if consuming the json, > I expect you will not want code that has to iterate over three > separate arrays. Just looking for some feedback really. The original > split > made sense for screen use, just not so sure about machine readability. > This issue also applies to ospfctl sh data, which returns 3 lists. A lot of this is depending on the imsgs used between ospfctl and ospfd. IIRC ospfd sends all data in one batch so the multiple lists just happen by ospfctl and indeed for json output it would be best to display the rib as a single array of entries (which have a similar structure but probably per LSA specific attributes). > When I am finished, should I just post the diff on here? Just > conscious it is quite a big refactor, albeit much of the code is > reused just moved into output.c as > was done for bgpctl. Please split it up like I did it for bgpctl. I first did a lot of the moving and cleanup and only later added the new input mode. This makes the individual steps smaller to review. > I am testing each call individually against a set of openbsd 6.6 boxes > we have running, which is great for ospfd. What is the normal > practise for ospf6, is there a script run to replicate code changes or > is it just a case of making very similar changes line by line? There is no script, you do it by hand it is quicker. > Just looking for the general thinking and approach I guess. I don't > currently have ospf6 set-up so that would be some overhead. > Happy to configure it though if needed/expected. Get ospfctl first, after that ospf6ctl can be done (and maybe somebody else will take care of that). > Finally what was the driver under bgpctl for the json output, ours is > for reading metrics to populate telemetry, just interested as the > purpose other > people have. Having this context will help in micro decisions when > implementing the json structure. I'm doing this work to provide JSON payloads to external looking glasses. I would not put the RIB into telemetry (that is just useless churn and load on the telemetry system). In bgpctl the terse outputs are simple space separated outputs for telemetry scripting but I guess people will also use the JSON output for this. > As a final thought is this something that is actually wanted > generally, I assumed it was as bgpctl has gone/is going in that > direction, but just don't want to assume. I see no reason why not. At least I wont veto it. -- :wq Claudio
ospfctl json support
Hi, I have done some work over the last few days to implement json support into ospfctl following the work done recently in bgpctl. I have some queries, hoping to get some help with. The change involves a refactor of ospfctl, but reuses the recent json.c written by Claudio, that is in the usr.sbin/bgpctl directory. At present no changes have been required at all. What is the best approach here, should/could this be centralised somewhere? In some cases there is room for change, for example ospfctl sh rib & ospfctl sh rib detail. In my view here, it makes sense to have a full list returned rather than splitting json into multiple lists of Router, Network and External. I looked for inspiration in the bgpctl, but couldn't find a similar pattern. The reason for a single list is that if consuming the json, I expect you will not want code that has to iterate over three separate arrays. Just looking for some feedback really. The original split made sense for screen use, just not so sure about machine readability. This issue also applies to ospfctl sh data, which returns 3 lists. When I am finished, should I just post the diff on here? Just conscious it is quite a big refactor, albeit much of the code is reused just moved into output.c as was done for bgpctl. I am testing each call individually against a set of openbsd 6.6 boxes we have running, which is great for ospfd. What is the normal practise for ospf6, is there a script run to replicate code changes or is it just a case of making very similar changes line by line? Just looking for the general thinking and approach I guess. I don't currently have ospf6 set-up so that would be some overhead. Happy to configure it though if needed/expected. Finally what was the driver under bgpctl for the json output, ours is for reading metrics to populate telemetry, just interested as the purpose other people have. Having this context will help in micro decisions when implementing the json structure. As a final thought is this something that is actually wanted generally, I assumed it was as bgpctl has gone/is going in that direction, but just don't want to assume. Cheers Rich
