On Thu, Jun 16, 2022 at 09:01:33PM +0200, Theo Buehler wrote:
> On Thu, Jun 16, 2022 at 07:15:51PM +0200, Claudio Jeker wrote:
> > Not much holds us back to switch kroute_find() to use a struct bgpd_addr
> > as prefix argument. Only the match code needs some adoption.
> > I created applymask() for this which works on bgpd_addrs like
> > inet4applymask works on in_addrs.
> >
> > I also switched a simple case in session.c over to applymask().
> > There is another one in bgpctl which I will send out once this is in.
>
> ok
>
> A nit and a question below.
>
> [...]
>
> > +kroute_find(struct ktable *kt, const struct bgpd_addr *prefix,
> > + uint8_t prefixlen, uint8_t prio)
> > {
> > struct kroute_node s;
> > struct kroute_node *kn, *tmp;
> >
> > - s.r.prefix.s_addr = prefix;
> > + s.r.prefix= prefix->v4;
>
> Missing space before =.
>
> [...]
>
> > Index: util.c
> > ===================================================================
> > RCS file: /cvs/src/usr.sbin/bgpd/util.c,v
> > retrieving revision 1.64
> > diff -u -p -r1.64 util.c
> > --- util.c 16 Jun 2022 15:33:05 -0000 1.64
> > +++ util.c 16 Jun 2022 16:50:47 -0000
> > @@ -783,6 +783,26 @@ inet6applymask(struct in6_addr *dest, co
> > dest->s6_addr[i] = src->s6_addr[i] & mask.s6_addr[i];
> > }
> >
> > +void
> > +applymask(struct bgpd_addr *dest, const struct bgpd_addr *src, int
> > prefixlen)
> > +{
> > + struct bgpd_addr tmp;
> > +
> > + /* use temporary storage in case src and dest point to same struct */
> > + tmp = *src;
>
> While I'm fine with doing it this way, why is this tmp dance needed? I
> would have thought that both inet4applymask() and inet6applymask() work
> fine if src and dest point to the same struct. bgpctl's parse.y assumes
> that this is the case.
While inet4applymask() and inet6applymask() work the problem comes with the
rest of the struct bgpd_addr fields.
Now I could do a trick and copy from offsetof(rd) to sizeof(*src) -
offsetof(rd) plus in the IPv4 case the rest of the union ba needs to
be cleared as well. So I think while possible to do this without this
temporary storage the code will be rather complex.
An alternative option is to just store the v4/v6 values before
memmove(dst, src) and then using those values in the inetXapplymask calls.
I guess that is a reasonable alternative which is a bit more optimized.
What do you think?
> > + switch (src->aid) {
> > + case AID_INET:
> > + case AID_VPN_IPv4:
> > + inet4applymask(&tmp.v4, &src->v4, prefixlen);
> > + break;
> > + case AID_INET6:
> > + case AID_VPN_IPv6:
> > + inet6applymask(&tmp.v6, &src->v6, prefixlen);
> > + break;
> > + }
> > + *dest = tmp;
> > +}
> > +
> > /* address family translation functions */
> > const struct aid aid_vals[AID_MAX] = AID_VALS;
> >
> >
>
--
:wq Claudio
