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