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.
> > log_warnx("RB_REMOVE(kif_tree, &kit, kif)");
> > return (-1);
> > }
--
:wq Claudio
