Martin Pieuchot([email protected]) on 2015.09.13 16:08:50 +0200:
> On 13/09/15(Sun) 15:51, Alexander Bluhm wrote:
> > On Sun, Sep 13, 2015 at 11:15:50AM +0200, Martin Pieuchot wrote:
> > > This makes the kernel simpler as it no longer try to find a new ifa
> > > when a route with a stale address is being used.
> >
> > This makes the code simpler, which is good.
> >
> > I am still not convinced that we want to loose the feature that the
> > routes jump to another interface address. When we have multiple
> > suiteable addresses and one gets deleted, the system can use another
> > one.
>
> This is the price to pay for making the code simpler. I strongly
> believe this "feature" is a side effect of history that should not
> have been added in the first place.
>
> However I'd like to fix potential issues with this diff before committing
> it, so tests are welcome :)
Hi,
i found the time to play with your diff.
On a router (where you probably wouldn't to this operationaly) you get tons
of "route xxx vanished before delete" from bgpd and ospfd, but they continue
to work, apparently as intended.
I havent found a box with pppoe to test. it would be nice if someone could
test that.
as far as it goes, ok from me.
/Benno
> > The patch itself looks correct. Just one question:
> >
> > > @@ -850,22 +850,10 @@ rtrequest1(int req, struct rt_addrinfo *
> > > return (EINVAL);
> > > if ((rt->rt_flags & RTF_CLONING) == 0)
> > > return (EINVAL);
> > > - if (rt->rt_ifa->ifa_ifp) {
> > > - info->rti_ifa = rt->rt_ifa;
> > > - } else {
> > > - /*
> > > - * The address of the cloning route is not longer
> > > - * configured on an interface, but its descriptor
> > > - * is still there because of reference counting.
> > > - *
> > > - * Try to find a similar active address and use
> > > - * it for the cloned route. The cloning route
> > > - * will get the new address and interface later.
> > > - */
> > > - info->rti_ifa = NULL;
> > > - info->rti_info[RTAX_IFA] = rt->rt_ifa->ifa_addr;
> > > - }
> > > + if (rt->rt_ifa->ifa_ifp == NULL)
> > > + return (EAGAIN);
> >
> > Why return EAGAIN here? Should it be EINVAL like in the other
> > cases? Can this happen at all?
>
> This should never happen but I'd like to take a safe approach and be
> able to differentiate the error code if this still happens. I'd
> happily turn this into a KASSERT() but not right now.
+1
