On Thursday, 07.05.2015 at 14:48, Antti Kantee wrote: > One problem is that as of very recently in NetBSD, "nam" is a > "struct sockaddr" instead of a "struct mbuf". So the code will not > work after I update src-netbsd. This is of course not your fault, > but I suggest we don't commit the code against the old interface at > all. I'll try to update src-netbsd tonight (I was planning to > bundle in some other changes with the update, but I think that plan > is from several weeks ago :/)
Ok, it can wait until you're done with the updates after which I'll rework the code. However, I don't see the NFS code on nxr.netbsd.org using "nam" as a "struct sockaddr": http://nxr.netbsd.org/source/xref/src/sys/nfs/nfs_bootdhcp.c#585 > ip6_var.h instead of manual extern? Ack. Didn't know about it. > "oneshot" in the ipv4 version means that it does not properly > implement leases. Does that apply here too? I don't think so, > right? No, I borrowed the term to mean "sends an RS packet and does not wait to see if an RA arrived". Ie. no feedback on whether or not the interface did get configured. Ideally we'd wait for an RA, wait to see the kernel actually configured an address and tell the user what it is. > >+ struct ifnet *ifp = NULL; > > why = NULL? I have a habit of (probably needlessly) over-initialising everything. I think its from the olden days when your bss wasn't zeroed. So, no reason :-) > >+ return ENODEV; > > ENXIO is the correct errno. Ack. > >+ if (ifp->if_sadl->sdl_type != IFT_ETHER) > >+ return EINVAL; > > Might as well consistently "goto out" in case someone reorganizes > the code. I realize you'd have to add braces and assign rv, but > c'est le code. Will do. > >+ nam = m_get(M_WAIT, MT_SONAME); > >+ sin6 = mtod(nam, struct sockaddr_in6 *); > > I forget what the rules are exactly, but you might technically need > to m_pulldown() before you're allowed to mtod for such a large > structure. Should no longer be relevant if I understand what you're saying about nam now being a "struct sockaddr"? > > >+ sin6->sin6_len = nam->m_len = sizeof (*sin6); > > Don't you need to set nam->m_pkthdr.len? The nfs code doesn't.... > KASSERT() that rslen <= MCLBYTES is a good idea. Actually, can be a > CTASSERT() Ok. > >+ rs = (struct nd_router_solicit *)buf; > >+ rs->nd_rs_type = ND_ROUTER_SOLICIT; > >+ buf += sizeof (*rs); > >+ opt = (struct nd_opt_hdr *)buf; > > Is "opt" *guaranteed* to be properly aligned? If yes, you can do it > that way. If no, you need to memcpy to the final space. Probably > safer to memcpy(). sizeof (struct nd_router_solicit) is 8, so should be fine. > >+ memcpy(buf, ifp->if_sadl->sdl_data + ifp->if_sadl->sdl_nlen, > >+ ETHER_ADDR_LEN); > > Use CLLADDR() Ack. Tried to use LLADDR() (translating from user code), didn't work :-) > >+ if (rv != 0) > >+ goto out; > >+ m = nam = NULL; > >+ rv = 0; > > So you fix these 4 lines to m = nam = NULL; Not sure what you mean here? Thanks for the review! -mato
