Re: bgpd more kroute cleanup
On Thu, Aug 18, 2022 at 12:44:07PM +0200, Claudio Jeker wrote: > It makes no sense to pass the fd to send_rtmsg() as an argument. > The code just passes the fd from the global kr_state. It also makes the > code less portable because for linux an mnl handle needs to be passed. > By dropping this the code becomes simpler. Makes sense ok
bgpd more kroute cleanup
It makes no sense to pass the fd to send_rtmsg() as an argument.
The code just passes the fd from the global kr_state. It also makes the
code less portable because for linux an mnl handle needs to be passed.
By dropping this the code becomes simpler.
--
:wq Claudio
Index: kroute.c
===
RCS file: /cvs/src/usr.sbin/bgpd/kroute.c,v
retrieving revision 1.292
diff -u -p -r1.292 kroute.c
--- kroute.c17 Aug 2022 15:15:26 - 1.292
+++ kroute.c18 Aug 2022 10:41:19 -
@@ -175,7 +175,7 @@ voidget_rtaddrs(int, struct sockaddr *
void if_change(u_short, int, struct if_data *);
void if_announce(void *);
-intsend_rtmsg(int, int, struct ktable *, struct kroute_full *);
+intsend_rtmsg(int, struct ktable *, struct kroute_full *);
intdispatch_rtmsg(void);
intfetchtable(struct ktable *);
intfetchifs(int);
@@ -497,7 +497,7 @@ kr4_change(struct ktable *kt, struct kro
else
kr->flags &= ~F_REJECT;
- if (send_rtmsg(kr_state.fd, RTM_CHANGE, kt, kf))
+ if (send_rtmsg(RTM_CHANGE, kt, kf))
kr->flags |= F_BGPD_INSERTED;
}
@@ -535,7 +535,7 @@ kr6_change(struct ktable *kt, struct kro
else
kr6->flags &= ~F_REJECT;
- if (send_rtmsg(kr_state.fd, RTM_CHANGE, kt, kf))
+ if (send_rtmsg(RTM_CHANGE, kt, kf))
kr6->flags |= F_BGPD_INSERTED;
}
@@ -587,7 +587,7 @@ krVPN4_change(struct ktable *kt, struct
else
kr->flags &= ~F_REJECT;
- if (send_rtmsg(kr_state.fd, RTM_CHANGE, kt, kf))
+ if (send_rtmsg(RTM_CHANGE, kt, kf))
kr->flags |= F_BGPD_INSERTED;
}
@@ -640,7 +640,7 @@ krVPN6_change(struct ktable *kt, struct
else
kr6->flags &= ~F_REJECT;
- if (send_rtmsg(kr_state.fd, RTM_CHANGE, kt, kf))
+ if (send_rtmsg(RTM_CHANGE, kt, kf))
kr6->flags |= F_BGPD_INSERTED;
}
@@ -714,14 +714,12 @@ kr_fib_couple(u_int rtableid)
RB_FOREACH(kr, kroute_tree, &kt->krt)
if (kr->flags & F_BGPD) {
- if (send_rtmsg(kr_state.fd, RTM_ADD, kt,
- kr_tofull(kr)))
+ if (send_rtmsg(RTM_ADD, kt, kr_tofull(kr)))
kr->flags |= F_BGPD_INSERTED;
}
RB_FOREACH(kr6, kroute6_tree, &kt->krt6)
if (kr6->flags & F_BGPD) {
- if (send_rtmsg(kr_state.fd, RTM_ADD, kt,
- kr6_tofull(kr6)))
+ if (send_rtmsg(RTM_ADD, kt, kr6_tofull(kr6)))
kr6->flags |= F_BGPD_INSERTED;
}
log_info("kernel routing table %u (%s) coupled", kt->rtableid,
@@ -752,14 +750,12 @@ kr_fib_decouple(u_int rtableid)
RB_FOREACH(kr, kroute_tree, &kt->krt)
if ((kr->flags & F_BGPD_INSERTED)) {
- if (send_rtmsg(kr_state.fd, RTM_DELETE, kt,
- kr_tofull(kr)))
+ if (send_rtmsg(RTM_DELETE, kt, kr_tofull(kr)))
kr->flags &= ~F_BGPD_INSERTED;
}
RB_FOREACH(kr6, kroute6_tree, &kt->krt6)
if ((kr6->flags & F_BGPD_INSERTED)) {
- if (send_rtmsg(kr_state.fd, RTM_DELETE, kt,
- kr6_tofull(kr6)))
+ if (send_rtmsg(RTM_DELETE, kt, kr6_tofull(kr6)))
kr6->flags &= ~F_BGPD_INSERTED;
}
@@ -1655,7 +1651,7 @@ kroute_insert(struct ktable *kt, struct
}
if (kf->flags & F_BGPD)
- if (send_rtmsg(kr_state.fd, RTM_ADD, kt, kf))
+ if (send_rtmsg(RTM_ADD, kt, kf))
kr->flags |= F_BGPD_INSERTED;
break;
case AID_INET6:
@@ -1691,7 +1687,7 @@ kroute_insert(struct ktable *kt, struct
}
if (kf->flags & F_BGPD)
- if (send_rtmsg(kr_state.fd, RTM_ADD, kt, kf))
+ if (send_rtmsg(RTM_ADD, kt, kf))
kr6->flags |= F_BGPD_INSERTED;
break;
}
@@ -1873,7 +1869,7 @@ kroute_remove(struct ktable *kt, struct
return (multipath + 1);
if (kf->flags & F_BGPD_INSERTED)
- send_rtmsg(kr_state.fd, RTM_DELETE, kt, kf);
+ send_rtmsg(RTM_DELETE, kt, kf);
/* remove only once all multipath routes are gone */
if (!(kf->flags & F_BGPD) && !multipath)
@@ -2622,7 +2618,7 @@ get_mpe_config(const char *name, u_int *
Re: bgpd more kroute cleanup
On Tue, Jun 14, 2022 at 08:14:45PM +0200, Theo Buehler wrote:
> On Tue, Jun 14, 2022 at 06:36:13PM +0200, Claudio Jeker wrote:
> > This diff does introduce a new flag for routes that have been inserted
> > into the FIB. Now I actually renamed the F_BGPD_INSERTED flag to F_BGPD
> > and reused F_BGPD_INSERTED as this new flag.
> > Adjust and use the send_rtmsg() return value to set F_BGPD_INSERTED if the
> > route message was successfully sent.
> >
> > Additionally this removes F_DYNAMIC and tracking of dynamic routes (routes
> > generated by the kernel because of PMTU or some other event). A routing
> > protocol does not need to work with them so just filter them out like ARP
> > and ND routes.
> >
> > While there also just remove protect_lo() it is a function that is not
> > really making anything better. There are other checks in place for
> > 127.0.0.1 and ::1.
>
> Would it not be cleaner to set or unset the F_BGPD_INSERTED flag
> inside send_rt{,6}msg() depending on action instead of doing that
> after inspecting the return code?
I decided against this because send_rtmsg() should not alter the kroute
node. In the end I want the kroute code to be a module and have the rtsock
code to be just a few hooks. This should make the code a lot more portable
with far less maintenance work.
> ok tb
>
> >
> > --
> > :wq Claudio
> >
> > Index: bgpctl/bgpctl.c
> > ===
> > RCS file: /cvs/src/usr.sbin/bgpctl/bgpctl.c,v
> > retrieving revision 1.276
> > diff -u -p -r1.276 bgpctl.c
> > --- bgpctl/bgpctl.c 21 Mar 2022 10:16:23 - 1.276
> > +++ bgpctl/bgpctl.c 14 Jun 2022 15:31:59 -
> > @@ -633,14 +633,12 @@ fmt_fib_flags(uint16_t flags)
> > else
> > strlcpy(buf, "*", sizeof(buf));
> >
> > - if (flags & F_BGPD_INSERTED)
> > + if (flags & F_BGPD)
> > strlcat(buf, "B", sizeof(buf));
> > else if (flags & F_CONNECTED)
> > strlcat(buf, "C", sizeof(buf));
> > else if (flags & F_STATIC)
> > strlcat(buf, "S", sizeof(buf));
> > - else if (flags & F_DYNAMIC)
> > - strlcat(buf, "D", sizeof(buf));
> > else
> > strlcat(buf, " ", sizeof(buf));
> >
> > Index: bgpctl/output.c
> > ===
> > RCS file: /cvs/src/usr.sbin/bgpctl/output.c,v
> > retrieving revision 1.20
> > diff -u -p -r1.20 output.c
> > --- bgpctl/output.c 6 Feb 2022 09:52:32 - 1.20
> > +++ bgpctl/output.c 14 Jun 2022 15:51:49 -
> > @@ -46,7 +46,7 @@ show_head(struct parse_result *res)
> > break;
> > case SHOW_FIB:
> > printf("flags: * = valid, B = BGP, C = Connected, "
> > - "S = Static, D = Dynamic\n");
> > + "S = Static\n");
> > printf(" "
> > "N = BGP Nexthop reachable via this route\n");
> > printf(" r = reject route, b = blackhole route\n\n");
> > Index: bgpctl/output_json.c
> > ===
> > RCS file: /cvs/src/usr.sbin/bgpctl/output_json.c,v
> > retrieving revision 1.14
> > diff -u -p -r1.14 output_json.c
> > --- bgpctl/output_json.c21 Mar 2022 10:16:23 - 1.14
> > +++ bgpctl/output_json.c14 Jun 2022 15:32:05 -
> > @@ -357,14 +357,12 @@ json_fib(struct kroute_full *kf)
> > json_do_printf("prefix", "%s/%u", log_addr(&kf->prefix), kf->prefixlen);
> > json_do_uint("priority", kf->priority);
> > json_do_bool("up", !(kf->flags & F_DOWN));
> > - if (kf->flags & F_BGPD_INSERTED)
> > + if (kf->flags & F_BGPD)
> > origin = "bgp";
> > else if (kf->flags & F_CONNECTED)
> > origin = "connected";
> > else if (kf->flags & F_STATIC)
> > origin = "static";
> > - else if (kf->flags & F_DYNAMIC)
> > - origin = "dynamic";
> > else
> > origin = "unknown";
> > json_do_printf("origin", "%s", origin);
> > Index: bgpctl/parser.c
> > ===
> > RCS file: /cvs/src/usr.sbin/bgpctl/parser.c,v
> > retrieving revision 1.109
> > diff -u -p -r1.109 parser.c
> > --- bgpctl/parser.c 21 Mar 2022 10:16:23 - 1.109
> > +++ bgpctl/parser.c 14 Jun 2022 15:29:05 -
> > @@ -151,15 +151,15 @@ static const struct token t_show_summary
> > };
> >
> > static const struct token t_show_fib[] = {
> > - { NOTOKEN, "", NONE,NULL},
> > - { FLAG, "connected",F_CONNECTED, t_show_fib},
> > - { FLAG, "static", F_STATIC,t_show_fib},
> > - { FLAG, "bgp", F_BGPD_INSERTED, t_show_fib},
> > - { FLAG, "nexthop", F_NEXTHOP, t_show_fib},
> > - { KEYWORD, "table",NONE,t_show_fib_table},
> > - { FAMILY, "", NONE,t_show_fib},
> > - { ADDRESS, "", NON
Re: bgpd more kroute cleanup
On Tue, Jun 14, 2022 at 06:36:13PM +0200, Claudio Jeker wrote:
> This diff does introduce a new flag for routes that have been inserted
> into the FIB. Now I actually renamed the F_BGPD_INSERTED flag to F_BGPD
> and reused F_BGPD_INSERTED as this new flag.
> Adjust and use the send_rtmsg() return value to set F_BGPD_INSERTED if the
> route message was successfully sent.
>
> Additionally this removes F_DYNAMIC and tracking of dynamic routes (routes
> generated by the kernel because of PMTU or some other event). A routing
> protocol does not need to work with them so just filter them out like ARP
> and ND routes.
>
> While there also just remove protect_lo() it is a function that is not
> really making anything better. There are other checks in place for
> 127.0.0.1 and ::1.
Would it not be cleaner to set or unset the F_BGPD_INSERTED flag
inside send_rt{,6}msg() depending on action instead of doing that
after inspecting the return code?
ok tb
>
> --
> :wq Claudio
>
> Index: bgpctl/bgpctl.c
> ===
> RCS file: /cvs/src/usr.sbin/bgpctl/bgpctl.c,v
> retrieving revision 1.276
> diff -u -p -r1.276 bgpctl.c
> --- bgpctl/bgpctl.c 21 Mar 2022 10:16:23 - 1.276
> +++ bgpctl/bgpctl.c 14 Jun 2022 15:31:59 -
> @@ -633,14 +633,12 @@ fmt_fib_flags(uint16_t flags)
> else
> strlcpy(buf, "*", sizeof(buf));
>
> - if (flags & F_BGPD_INSERTED)
> + if (flags & F_BGPD)
> strlcat(buf, "B", sizeof(buf));
> else if (flags & F_CONNECTED)
> strlcat(buf, "C", sizeof(buf));
> else if (flags & F_STATIC)
> strlcat(buf, "S", sizeof(buf));
> - else if (flags & F_DYNAMIC)
> - strlcat(buf, "D", sizeof(buf));
> else
> strlcat(buf, " ", sizeof(buf));
>
> Index: bgpctl/output.c
> ===
> RCS file: /cvs/src/usr.sbin/bgpctl/output.c,v
> retrieving revision 1.20
> diff -u -p -r1.20 output.c
> --- bgpctl/output.c 6 Feb 2022 09:52:32 - 1.20
> +++ bgpctl/output.c 14 Jun 2022 15:51:49 -
> @@ -46,7 +46,7 @@ show_head(struct parse_result *res)
> break;
> case SHOW_FIB:
> printf("flags: * = valid, B = BGP, C = Connected, "
> - "S = Static, D = Dynamic\n");
> + "S = Static\n");
> printf(" "
> "N = BGP Nexthop reachable via this route\n");
> printf(" r = reject route, b = blackhole route\n\n");
> Index: bgpctl/output_json.c
> ===
> RCS file: /cvs/src/usr.sbin/bgpctl/output_json.c,v
> retrieving revision 1.14
> diff -u -p -r1.14 output_json.c
> --- bgpctl/output_json.c 21 Mar 2022 10:16:23 - 1.14
> +++ bgpctl/output_json.c 14 Jun 2022 15:32:05 -
> @@ -357,14 +357,12 @@ json_fib(struct kroute_full *kf)
> json_do_printf("prefix", "%s/%u", log_addr(&kf->prefix), kf->prefixlen);
> json_do_uint("priority", kf->priority);
> json_do_bool("up", !(kf->flags & F_DOWN));
> - if (kf->flags & F_BGPD_INSERTED)
> + if (kf->flags & F_BGPD)
> origin = "bgp";
> else if (kf->flags & F_CONNECTED)
> origin = "connected";
> else if (kf->flags & F_STATIC)
> origin = "static";
> - else if (kf->flags & F_DYNAMIC)
> - origin = "dynamic";
> else
> origin = "unknown";
> json_do_printf("origin", "%s", origin);
> Index: bgpctl/parser.c
> ===
> RCS file: /cvs/src/usr.sbin/bgpctl/parser.c,v
> retrieving revision 1.109
> diff -u -p -r1.109 parser.c
> --- bgpctl/parser.c 21 Mar 2022 10:16:23 - 1.109
> +++ bgpctl/parser.c 14 Jun 2022 15:29:05 -
> @@ -151,15 +151,15 @@ static const struct token t_show_summary
> };
>
> static const struct token t_show_fib[] = {
> - { NOTOKEN, "", NONE,NULL},
> - { FLAG, "connected",F_CONNECTED, t_show_fib},
> - { FLAG, "static", F_STATIC,t_show_fib},
> - { FLAG, "bgp", F_BGPD_INSERTED, t_show_fib},
> - { FLAG, "nexthop", F_NEXTHOP, t_show_fib},
> - { KEYWORD, "table",NONE,t_show_fib_table},
> - { FAMILY, "", NONE,t_show_fib},
> - { ADDRESS, "", NONE,NULL},
> - { ENDTOKEN, "", NONE,NULL}
> + { NOTOKEN, "", NONE, NULL},
> + { FLAG, "connected",F_CONNECTED,t_show_fib},
> + { FLAG, "static", F_STATIC, t_show_fib},
> + { FLAG, "bgp", F_BGPD, t_show_fib},
> + { FLAG, "nexthop", F_NEXTHOP, t_show_fib},
> +
bgpd more kroute cleanup
This diff does introduce a new flag for routes that have been inserted
into the FIB. Now I actually renamed the F_BGPD_INSERTED flag to F_BGPD
and reused F_BGPD_INSERTED as this new flag.
Adjust and use the send_rtmsg() return value to set F_BGPD_INSERTED if the
route message was successfully sent.
Additionally this removes F_DYNAMIC and tracking of dynamic routes (routes
generated by the kernel because of PMTU or some other event). A routing
protocol does not need to work with them so just filter them out like ARP
and ND routes.
While there also just remove protect_lo() it is a function that is not
really making anything better. There are other checks in place for
127.0.0.1 and ::1.
--
:wq Claudio
Index: bgpctl/bgpctl.c
===
RCS file: /cvs/src/usr.sbin/bgpctl/bgpctl.c,v
retrieving revision 1.276
diff -u -p -r1.276 bgpctl.c
--- bgpctl/bgpctl.c 21 Mar 2022 10:16:23 - 1.276
+++ bgpctl/bgpctl.c 14 Jun 2022 15:31:59 -
@@ -633,14 +633,12 @@ fmt_fib_flags(uint16_t flags)
else
strlcpy(buf, "*", sizeof(buf));
- if (flags & F_BGPD_INSERTED)
+ if (flags & F_BGPD)
strlcat(buf, "B", sizeof(buf));
else if (flags & F_CONNECTED)
strlcat(buf, "C", sizeof(buf));
else if (flags & F_STATIC)
strlcat(buf, "S", sizeof(buf));
- else if (flags & F_DYNAMIC)
- strlcat(buf, "D", sizeof(buf));
else
strlcat(buf, " ", sizeof(buf));
Index: bgpctl/output.c
===
RCS file: /cvs/src/usr.sbin/bgpctl/output.c,v
retrieving revision 1.20
diff -u -p -r1.20 output.c
--- bgpctl/output.c 6 Feb 2022 09:52:32 - 1.20
+++ bgpctl/output.c 14 Jun 2022 15:51:49 -
@@ -46,7 +46,7 @@ show_head(struct parse_result *res)
break;
case SHOW_FIB:
printf("flags: * = valid, B = BGP, C = Connected, "
- "S = Static, D = Dynamic\n");
+ "S = Static\n");
printf(" "
"N = BGP Nexthop reachable via this route\n");
printf(" r = reject route, b = blackhole route\n\n");
Index: bgpctl/output_json.c
===
RCS file: /cvs/src/usr.sbin/bgpctl/output_json.c,v
retrieving revision 1.14
diff -u -p -r1.14 output_json.c
--- bgpctl/output_json.c21 Mar 2022 10:16:23 - 1.14
+++ bgpctl/output_json.c14 Jun 2022 15:32:05 -
@@ -357,14 +357,12 @@ json_fib(struct kroute_full *kf)
json_do_printf("prefix", "%s/%u", log_addr(&kf->prefix), kf->prefixlen);
json_do_uint("priority", kf->priority);
json_do_bool("up", !(kf->flags & F_DOWN));
- if (kf->flags & F_BGPD_INSERTED)
+ if (kf->flags & F_BGPD)
origin = "bgp";
else if (kf->flags & F_CONNECTED)
origin = "connected";
else if (kf->flags & F_STATIC)
origin = "static";
- else if (kf->flags & F_DYNAMIC)
- origin = "dynamic";
else
origin = "unknown";
json_do_printf("origin", "%s", origin);
Index: bgpctl/parser.c
===
RCS file: /cvs/src/usr.sbin/bgpctl/parser.c,v
retrieving revision 1.109
diff -u -p -r1.109 parser.c
--- bgpctl/parser.c 21 Mar 2022 10:16:23 - 1.109
+++ bgpctl/parser.c 14 Jun 2022 15:29:05 -
@@ -151,15 +151,15 @@ static const struct token t_show_summary
};
static const struct token t_show_fib[] = {
- { NOTOKEN, "", NONE,NULL},
- { FLAG, "connected",F_CONNECTED, t_show_fib},
- { FLAG, "static", F_STATIC,t_show_fib},
- { FLAG, "bgp", F_BGPD_INSERTED, t_show_fib},
- { FLAG, "nexthop", F_NEXTHOP, t_show_fib},
- { KEYWORD, "table",NONE,t_show_fib_table},
- { FAMILY, "", NONE,t_show_fib},
- { ADDRESS, "", NONE,NULL},
- { ENDTOKEN, "", NONE,NULL}
+ { NOTOKEN, "", NONE, NULL},
+ { FLAG, "connected",F_CONNECTED,t_show_fib},
+ { FLAG, "static", F_STATIC, t_show_fib},
+ { FLAG, "bgp", F_BGPD, t_show_fib},
+ { FLAG, "nexthop", F_NEXTHOP, t_show_fib},
+ { KEYWORD, "table",NONE, t_show_fib_table},
+ { FAMILY, "", NONE, t_show_fib},
+ { ADDRESS, "", NONE, NULL},
+ { ENDTOKEN, "", NONE, NULL}
};
static const struct token t_show_rib[] = {
Index: bgpd/bgp
