On Fri, Jun 17, 2022 at 12:32:50PM +0200, Theo Buehler wrote:
> On Fri, Jun 17, 2022 at 11:37:29AM +0200, Claudio Jeker wrote:
> > 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?
>
> While I'm not against your alternative option, I think the copies are
> cheap enough that we should not need to optimize them.
>
> What confuses me is the comment and why we need temporary storage at
> all.
>
> The simple assignment is well-defined if src and dest point to the same
> struct (https://port70.net/%7Ensz/c/c99/n1256.html#6.5.16.1p3), so I
> would have written applymask() this way:
>
> void
> applymask(struct bgpd_addr *dest, const struct bgpd_addr *src, int prefixlen)
> {
> *dest = *src;
> switch (src->aid) {
> case AID_INET:
> case AID_VPN_IPv4:
> inet4applymask(&dest->v4, &src->v4, prefixlen);
> break;
> case AID_INET6:
> case AID_VPN_IPv6:
> inet6applymask(&dest->v6, &src->v6, prefixlen);
> break;
> }
> }
>
Yes, I blame it on the heat that my brain made me think this is more
complex than it actually is. Indeed that should work. I changed my diff to
that version.
--
:wq Claudio
