On Tue, Jul 26, 2022 at 05:35:47PM +0200, Theo Buehler wrote:
> On Tue, Jul 26, 2022 at 05:17:23PM +0200, Claudio Jeker wrote:
> > On Tue, Jul 26, 2022 at 03:51:40PM +0200, Theo Buehler wrote:
> > > On Tue, Jul 26, 2022 at 03:09:37PM +0200, Claudio Jeker wrote:
> > > > This is another step in the epic kroute rework.
> > > >
> > > > Interfaces (kif) come with a list of kroutes attached to them which are
> > > > only used to track the interface state and to fiddle with nexthop
> > > > states.
> > > > Now these lists are not really needed. One can just validate the
> > > > nexthops
> > > > without losing any relevant information.
> > > >
> > > > Struct knexthop gains a ifindex member which is used to track link state
> > > > changes. As a benefit gateway routes will also pick up state changes now
> > > > (connected routes already did that).
> > > >
> > > > With this F_DOWN becomes unused and I will remove this in another step.
> > >
> > > This generally reads fine. One question:
> > >
> > > > @@ -2097,31 +2048,23 @@ int
> > > > kif_remove(struct kif_node *kif)
> > > > {
> > > > struct ktable *kt;
> > > > - struct kif_kr *kkr;
> > > > - struct kif_kr6 *kkr6;
> > > > +
> > > > + kif->flags &= ~IFF_UP;
> > > > +
> > > > + /*
> > > > + * TODO, remove all kroutes using this interface,
> > > > + * the kernel does this for us but better to do it
> > > > + * here as well.
> > > > + */
> > > > +
> > > > + if ((kt = ktable_get(kif->k.rdomain)) != NULL)
> > > > + knexthop_track(kt, kif->k.ifindex);
> > > >
> > > > if (RB_REMOVE(kif_tree, &kit, kif) == NULL) {
> > >
> > > If I'm not mistaken, the only way RB_REMOVE() can return NULL is when
> > > the third argument is NULL. So this check used to act as a NULL check
> > > before dereferencing kif. This diff adds two derefs earlier, which will
> > > now crash.
> > >
> > > Should kif_remove() in if_announce() only be called with non-NULL kif?
> >
> > I have never seen the log_warnx. So I assume it can not really happen.
> > Maybe remove the if check and just adjust if_announce instead.
> > I'll have a look and send another diff for that.
>
> Fine with me.
>
> ok
This is what I have in mind for that.
--
:wq Claudio
? obj
Index: kroute.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/kroute.c,v
retrieving revision 1.281
diff -u -p -r1.281 kroute.c
--- kroute.c 26 Jul 2022 16:36:33 -0000 1.281
+++ kroute.c 26 Jul 2022 16:37:17 -0000
@@ -2060,11 +2060,7 @@ kif_remove(struct kif_node *kif)
if ((kt = ktable_get(kif->k.rdomain)) != NULL)
knexthop_track(kt, kif->k.ifindex);
- if (RB_REMOVE(kif_tree, &kit, kif) == NULL) {
- log_warnx("RB_REMOVE(kif_tree, &kit, kif)");
- return (-1);
- }
-
+ RB_REMOVE(kif_tree, &kit, kif);
free(kif);
return (0);
}
@@ -2549,7 +2545,8 @@ if_announce(void *msg)
break;
case IFAN_DEPARTURE:
kif = kif_find(ifan->ifan_index);
- kif_remove(kif);
+ if (kif != NULL)
+ kif_remove(kif);
break;
}
}
