On Sun, Dec 10, 2006 at 02:58:39PM +0200, Antti Kantee wrote:
> Just skimming through the patch, how about making rtcache_init() return if
> the cache is valid.  It'd avoid a lot of rtcache_init(); if (!= NULL) foo;

I could do that, but I don't consider it a failure condition. No strong
opinion about it. See below for a comment why.

> Then (of course!), I find the naming confusing.  I'd expect _check()
> to return if the cache valid, not have side effects.

It checks that the entry is valid. If it is not valid -- there's no
reason to keep the old reference.

> Maybe call it
> _reheat()? (ok, equally sucky name, but at least less confusing ;)

I don't like that name more than the other. _revalidate or so is too
long IMO :-)

> Also,
> maybe _invalidate() instead of _free() and add isvalid() to avoid manual
> comparison against NULL.

NULL *is* a valid route as well. Part of the problem is that the
semantics are currently overloaded, a non-routable entry and an
invalidated entry are currently the same. I can't cleanup that mess
directly, as the conditions for invalidating are a bit more complicated.
Consider e.g. unbound UDP sockets, where an address change is another
condition for invalidation. That's also the reason why rtcache_check
does *not* relookup ro_dst, because the cases where a different address
might be of interest is also quite common. This will change later with
some related patches, but not now.

> And while you're abstracting, it would be nice if _init() could take
> the address to truly *init* the route cache (err.. or route, route not
> being rtentry.  gotta love the naming).  And then we'd just call _update()
> or _hopefully_soon_to_be_renamed_check() everywhere else.

It is planned to move the destination address out of struct route -- but
that is another huge patch. That's when I will rename struct route :-)
This is also a precondition for doing more intelligent updates. Before I
don't really know whether rtcache_init was ever called and the initial
iteration won't cover all cases. There's a lot of reliance on bzero
getting it right.

> The use of rtcache in pf seems excessive.  Maybe it would be better to
> fix the routing interface to allow such a query (yes, I know, it wasn't
> the point here, but this part just stuck out of the patch).

When the destination address is moved out of struct route, it becomes as
efficient as it will get. The reference count dance can't be avoided as
a route might later be removed during the PF processing.

Joerg