From: Claudio Jeker Subject: Re: route cache return hit or miss To: Alexander Bluhm Cc: tech@openbsd.org Date: Fri, 9 Feb 2024 12:35:49 +0100 On Fri, Feb 09, 2024 at 11:45:12AM +0100, Alexander Bluhm wrote: > 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. But route_cache() is a simple boolean function. There is no complexity in it. It eiter was a cache hit or it wasn't. > Functions that allocate should return pointer or NULL. > Boolean questions like rtisvalid() return 0 or 1. I prefer 0 and -1 for boolean returns. > 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. In any case this is just a bikeshed issue. OK claudio@ on the diff. > 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 > -- :wq Claudio