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;
}
}
>
> > > + 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
