On Wed, Nov 23, 2022 at 09:06:55AM +0000, Klemens Nanni wrote:
> On Wed, Nov 23, 2022 at 09:49:17AM +0100, Claudio Jeker wrote:
> > On Wed, Nov 23, 2022 at 08:24:11AM +0000, Klemens Nanni wrote:
> > > *if_afdata[] and struct domain's dom_if{at,de}tach() are only used with
> > > IPv6 Neighbour Discovery in6_dom{at,de}tach(), which allocate/init and
> > > free single struct nd_ifinfo.
> > >
> > > Set up a new ND-specific *if_nd member directly to avoid yet another
> > > layer of indirection and thus make the generic domain API obsolete.
> > >
> > > The per-interface data is only accessed in nd6.c and nd6_nbr.c through
> > > the ND_IFINFO() macro; it is allocated and freed exactly once during
> > > interface at/detach, so document it as [I]mmutable.
> > >
> > > Next up as separate commits:
> > > - remove *if_afdata[] struct domain's dom_if{at,de}tach()
> > > - nd6_if{at,de}tach() return/argument type cleanup
> > > - inline ND_IFINFO()
> > >
> > > Feedback? Objection? OK?
> >
> > I like this but one comment below.
> >
> > > diff --git a/sys/net/if.c b/sys/net/if.c
> > > index f3fba33de3f..bdaa41aea9f 100644
> > > --- a/sys/net/if.c
> > > +++ b/sys/net/if.c
> > > @@ -453,16 +453,25 @@ if_idxmap_remove(struct ifnet *ifp)
> > > */
> > > void
> > > if_attachsetup(struct ifnet *ifp)
> > > {
> > > unsigned long ifidx;
> > > +#ifdef INET6
> > > + int s;
> > > +#endif
> > >
> > > NET_ASSERT_LOCKED();
> > >
> > > if_addgroup(ifp, IFG_ALL);
> > >
> > > if_attachdomain(ifp);
> > > +#ifdef INET6
> > > + s = splnet();
> > > + ifp->if_nd = nd6_ifattach(ifp);
> > > + splx(s);
> >
> > There is no need to do this splnet() dance here.
> > nd6_ifattach calls malloc() and then sets some values of that memory and
> > returns the pointer. There is no need for splnet() around that code
> > nothing can see the returned value ahead of time.
>
> if_attachdomain(), where nd6_ifattach() is currently called, does this,
> so it just remained.
>
> I'll drop it, thanks, just didn't want to do too much stuff at once.
While I agree, I think bad code should not be copied. It makes the code
not more complex makes the diff shorter and is quite obvious.
Lets do this right.
> > Shouldn't we also remove if_afdata in the same commit? You killed the last
> > user of this and I don't like to alter somewhat common structs over and
> > over. Removeing this part of the code as well wont make this diff much
> > larger.
>
> I've removed it together with the dead domain API code in net/if.c
> as well as the two struct domain members.
>
> Moving stuff out of the API in one commit (this one) and then removing
> dead code afterwards seems clearer to me, but I can send one big diff if
> that's preferred.
I don't mind them to be two commits but please share both of them at the
same time. Because they should hit the tree at the same time. Changing
header files like net/if_var.h comes at a cost so don't let people suffer
twice for little reason.
--
:wq Claudio
