Download raw body.
route cache return hit or miss
On Fri, Feb 09, 2024 at 08:39:32AM +0100, Claudio Jeker wrote:
> On Thu, Feb 08, 2024 at 11:15:40PM +0100, Alexander Bluhm wrote:
> > Hi,
> >
> > The route_cache() function can easily return whether it was a cache
> > hit or miss. Then the logic to perform a route lookup gets a bit
> > simpler. Some more complicated if (ro->ro_rt == NULL) checks still
> > exist elsewhere.
> >
> > Also use route cache in in_pcbselsrc() instead of filling struct
> > route manually.
> >
> > ok?
>
> Why return ESRCH instead of simply -1?
I don't like functions that return 0, 1, or -1 in general. This
way you may reach the OpenSSL API design.
Functions that allocate should return pointer or NULL.
Boolean questions like rtisvalid() return 0 or 1.
All others either succeed or fail. Failure should be expressed by
an errno. Lookups usually return ESRCH if an object is not there.
Returning error number makes is more readble what is expected and
what happens in the other cases.
bluhm
> Apart from that I like this a lot.
>
> > Index: net/route.c
> > ===================================================================
> > RCS file: /data/mirror/openbsd/cvs/src/sys/net/route.c,v
> > diff -u -p -r1.430 route.c
> > --- net/route.c 7 Feb 2024 23:52:20 -0000 1.430
> > +++ net/route.c 8 Feb 2024 14:43:01 -0000
> > @@ -201,7 +201,7 @@ route_init(void)
> > #endif
> > }
> >
> > -void
> > +int
> > route_cache(struct route *ro, struct in_addr addr, u_int rtableid)
> > {
> > u_long gen;
> > @@ -215,7 +215,7 @@ route_cache(struct route *ro, struct in_
> > ro->ro_dst.sa_family == AF_INET &&
> > satosin(&ro->ro_dst)->sin_addr.s_addr == addr.s_addr) {
> > ipstat_inc(ips_rtcachehit);
> > - return;
> > + return (0);
> > }
> >
> > ipstat_inc(ips_rtcachemiss);
> > @@ -228,10 +228,12 @@ route_cache(struct route *ro, struct in_
> > satosin(&ro->ro_dst)->sin_family = AF_INET;
> > satosin(&ro->ro_dst)->sin_len = sizeof(struct sockaddr_in);
> > satosin(&ro->ro_dst)->sin_addr = addr;
> > +
> > + return (ESRCH);
> > }
> >
> > #ifdef INET6
> > -void
> > +int
> > route6_cache(struct route_in6 *ro, const struct in6_addr *addr,
> > u_int rtableid)
> > {
> > @@ -246,7 +248,7 @@ route6_cache(struct route_in6 *ro, const
> > ro->ro_dst.sin6_family == AF_INET6 &&
> > IN6_ARE_ADDR_EQUAL(&ro->ro_dst.sin6_addr, addr)) {
> > ip6stat_inc(ip6s_rtcachehit);
> > - return;
> > + return (0);
> > }
> >
> > ip6stat_inc(ip6s_rtcachemiss);
> > @@ -259,6 +261,8 @@ route6_cache(struct route_in6 *ro, const
> > ro->ro_dst.sin6_family = AF_INET6;
> > ro->ro_dst.sin6_len = sizeof(struct sockaddr_in6);
> > ro->ro_dst.sin6_addr = *addr;
> > +
> > + return (ESRCH);
> > }
> > #endif
> >
> > Index: netinet/in.h
> > ===================================================================
> > RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/in.h,v
> > diff -u -p -r1.146 in.h
> > --- netinet/in.h 5 Feb 2024 12:52:11 -0000 1.146
> > +++ netinet/in.h 8 Feb 2024 14:37:42 -0000
> > @@ -789,7 +789,7 @@ void in_len2mask(struct in_addr *, in
> > int in_nam2sin(const struct mbuf *, struct sockaddr_in **);
> > int in_sa2sin(struct sockaddr *, struct sockaddr_in **);
> >
> > -void route_cache(struct route *, struct in_addr, u_int);
> > +int route_cache(struct route *, struct in_addr, u_int);
> >
> > char *inet_ntoa(struct in_addr);
> > int inet_nat64(int, const void *, void *, const void *, u_int8_t);
> > Index: netinet/in_pcb.c
> > ===================================================================
> > RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/in_pcb.c,v
> > diff -u -p -r1.290 in_pcb.c
> > --- netinet/in_pcb.c 7 Feb 2024 23:40:40 -0000 1.290
> > +++ netinet/in_pcb.c 8 Feb 2024 14:44:49 -0000
> > @@ -918,8 +918,7 @@ in_pcbrtentry(struct inpcb *inp)
> >
> > if (inp->inp_faddr.s_addr == INADDR_ANY)
> > return (NULL);
> > - route_cache(ro, inp->inp_faddr, inp->inp_rtableid);
> > - if (ro->ro_rt == NULL) {
> > + if (route_cache(ro, inp->inp_faddr, inp->inp_rtableid)) {
> > ro->ro_rt = rtalloc_mpath(&ro->ro_dst,
> > &inp->inp_laddr.s_addr, ro->ro_tableid);
> > }
> > @@ -941,8 +940,6 @@ in_pcbselsrc(struct in_addr *insrc, stru
> > const struct in_addr *laddr = &inp->inp_laddr;
> > u_int rtableid = inp->inp_rtableid;
> > struct sockaddr *ip4_source = NULL;
> > -
> > - struct sockaddr_in *sin2;
> > struct in_ifaddr *ia = NULL;
> >
> > /*
> > @@ -984,25 +981,9 @@ in_pcbselsrc(struct in_addr *insrc, stru
> > * If route is known or can be allocated now,
> > * our src addr is taken from the i/f, else punt.
> > */
> > - if (!rtisvalid(ro->ro_rt) || (ro->ro_tableid != rtableid) ||
> > - (satosin(&ro->ro_dst)->sin_addr.s_addr != sin->sin_addr.s_addr)) {
> > - rtfree(ro->ro_rt);
> > - ro->ro_rt = NULL;
> > - }
> > - if (ro->ro_rt == NULL) {
> > + if (route_cache(ro, sin->sin_addr, rtableid)) {
> > /* No route yet, so try to acquire one */
> > - ro->ro_dst.sa_family = AF_INET;
> > - ro->ro_dst.sa_len = sizeof(struct sockaddr_in);
> > - satosin(&ro->ro_dst)->sin_addr = sin->sin_addr;
> > - ro->ro_tableid = rtableid;
> > ro->ro_rt = rtalloc_mpath(&ro->ro_dst, NULL, ro->ro_tableid);
> > -
> > - /*
> > - * It is important to zero out the rest of the
> > - * struct sockaddr_in when mixing v6 & v4!
> > - */
> > - sin2 = satosin(&ro->ro_dst);
> > - memset(sin2->sin_zero, 0, sizeof(sin2->sin_zero));
> > }
> >
> > /*
> > Index: netinet6/in6.h
> > ===================================================================
> > RCS file: /data/mirror/openbsd/cvs/src/sys/netinet6/in6.h,v
> > diff -u -p -r1.114 in6.h
> > --- netinet6/in6.h 7 Feb 2024 23:40:40 -0000 1.114
> > +++ netinet6/in6.h 8 Feb 2024 14:38:13 -0000
> > @@ -428,7 +428,7 @@ int in6_mask2len(struct in6_addr *, u_ch
> > int in6_nam2sin6(const struct mbuf *, struct sockaddr_in6 **);
> > int in6_sa2sin6(struct sockaddr *, struct sockaddr_in6 **);
> >
> > -void route6_cache(struct route_in6 *, const struct in6_addr *, u_int);
> > +int route6_cache(struct route_in6 *, const struct in6_addr *, u_int);
> >
> > struct ip6_pktopts;
> > struct ip6_moptions;
> > Index: netinet6/in6_pcb.c
> > ===================================================================
> > RCS file: /data/mirror/openbsd/cvs/src/sys/netinet6/in6_pcb.c,v
> > diff -u -p -r1.135 in6_pcb.c
> > --- netinet6/in6_pcb.c 7 Feb 2024 23:40:40 -0000 1.135
> > +++ netinet6/in6_pcb.c 8 Feb 2024 14:47:09 -0000
> > @@ -568,8 +568,7 @@ in6_pcbrtentry(struct inpcb *inp)
> >
> > if (IN6_IS_ADDR_UNSPECIFIED(&inp->inp_faddr6))
> > return (NULL);
> > - route6_cache(ro, &inp->inp_faddr6, inp->inp_rtableid);
> > - if (ro->ro_rt == NULL) {
> > + if (route6_cache(ro, &inp->inp_faddr6, inp->inp_rtableid)) {
> > ro->ro_rt = rtalloc_mpath(sin6tosa(&ro->ro_dst),
> > &inp->inp_laddr6.s6_addr32[0], ro->ro_tableid);
> > }
> > Index: netinet6/in6_src.c
> > ===================================================================
> > RCS file: /data/mirror/openbsd/cvs/src/sys/netinet6/in6_src.c,v
> > diff -u -p -r1.92 in6_src.c
> > --- netinet6/in6_src.c 7 Feb 2024 23:40:40 -0000 1.92
> > +++ netinet6/in6_src.c 8 Feb 2024 14:48:05 -0000
> > @@ -179,8 +179,7 @@ in6_pcbselsrc(const struct in6_addr **in
> > * If route is known or can be allocated now,
> > * our src addr is taken from the i/f, else punt.
> > */
> > - route6_cache(ro, dst, rtableid);
> > - if (ro->ro_rt == NULL) {
> > + if (route6_cache(ro, dst, rtableid)) {
> > ro->ro_rt = rtalloc(sin6tosa(&ro->ro_dst),
> > RT_RESOLVE, ro->ro_tableid);
> > }
> > @@ -306,8 +305,7 @@ in6_selectroute(const struct in6_addr *d
> > * a new one.
> > */
> > if (ro) {
> > - route6_cache(ro, dst, rtableid);
> > - if (ro->ro_rt == NULL) {
> > + if (route6_cache(ro, dst, rtableid)) {
> > /* No route yet, so try to acquire one */
> > ro->ro_rt = rtalloc_mpath(sin6tosa(&ro->ro_dst),
> > NULL, ro->ro_tableid);
> >
>
> --
> :wq Claudio
route cache return hit or miss