On 07/05/15 14:12, Martin Lucina wrote:
Hi,

I've implemented support for IPv6 SLAAC in brlib (and by extension,
rumprun).

I'll be the first to admit I didn't know what SLAAC is without ddg'ing it. But, awesome!

One thing I'm not sure about, I modeled the sending code on what
nfs_boot_sendrecv() does, am I right in understanding that if so_send()
completes successfully it takes ownership of the "nam" and "m" mbufs passed
to it?

I don't remember, but with my reading of both sosend() and nfs_boot_sendrecv(), calling (*so_send()) always transfers ownership.

Historic code sometimes does have these weird interfaces where the resource ownership is asymmetric depending on the return value, but I don't like that style because it's very error-prone. Luckily, this appears to not be one of those cases.

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 :/)

-mato

diff --git a/brlib/libnetconfig/netconfig.c b/brlib/libnetconfig/netconfig.c
index 7a33b0e..43d6aa2 100644
--- a/brlib/libnetconfig/netconfig.c
+++ b/brlib/libnetconfig/netconfig.c
@@ -28,9 +28,13 @@
  #include <sys/socketvar.h>

  #include <net/if.h>
+#include <net/if_dl.h>
+#include <net/if_ether.h>
+#include <net/if_types.h>
  #include <net/route.h>

  #include <netinet/in.h>
+#include <netinet/icmp6.h>

  #include <netinet6/in6.h>
  #include <netinet6/in6_var.h>
@@ -317,6 +321,93 @@ rump_netconfig_ipv6_gw(const char *gwaddr)
        return rv;
  }

+extern int ip6_accept_rtadv;

ip6_var.h instead of manual extern?

+ */
+int
+rump_netconfig_auto_ipv6_oneshot(const char *ifname)

"oneshot" in the ipv4 version means that it does not properly implement leases. Does that apply here too? I don't think so, right?

+       struct ifnet *ifp = NULL;

why = NULL?

+       int ifindex;
+       struct socket *rsso = NULL;
+       int rv = 0;
+       int hoplimit = 255;
+       struct mbuf *m = NULL,
+                   *nam = NULL;
+       struct sockaddr_in6 *sin6;
+       char *buf;
+       struct nd_router_solicit *rs;
+       struct nd_opt_hdr *opt;
+       size_t rslen;

+       ifp = ifunit(ifname);
+       if (ifp == NULL)
+               return ENODEV;

ENXIO is the correct errno.

+       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.

+       rv = socreate(PF_INET6, &rsso, SOCK_RAW, IPPROTO_ICMPV6, curlwp, NULL);
+       if (rv != 0)
+               goto out;
+       ifindex = ifp->if_index;
+       rv = so_setsockopt(curlwp, rsso, IPPROTO_IPV6, IPV6_MULTICAST_IF,
+                       &ifindex, sizeof ifindex);
+       if (rv != 0)
+               goto out;
+       rv = so_setsockopt(curlwp, rsso, IPPROTO_IPV6, IPV6_MULTICAST_HOPS,
+                       &hoplimit, sizeof hoplimit);
+       if (rv != 0)
+               goto out;
+
+       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.

+       sin6->sin6_len = nam->m_len = sizeof (*sin6);

Don't you need to set nam->m_pkthdr.len?

+       sin6->sin6_family = AF_INET6;
+       netconfig_inet_pton6("ff02::2", &sin6->sin6_addr);
+
+       rslen = sizeof (struct nd_router_solicit) +
+               sizeof (struct nd_opt_hdr) + ETHER_ADDR_LEN;
+       m = m_gethdr(M_WAIT, MT_DATA);
+       m_clget(m, M_WAIT);
+       m->m_pkthdr.len = m->m_len = rslen;
+       m->m_pkthdr.rcvif = NULL;
+       buf = mtod(m, char *);
+       memset (buf, 0, rslen);

KASSERT() that rslen <= MCLBYTES is a good idea. Actually, can be a CTASSERT()

+       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().

+       opt->nd_opt_type = ND_OPT_SOURCE_LINKADDR;
+       opt->nd_opt_len = 1; /* units of 8 octets */
+       buf += sizeof (*opt);
+       memcpy(buf, ifp->if_sadl->sdl_data + ifp->if_sadl->sdl_nlen,
+               ETHER_ADDR_LEN);

Use CLLADDR()

+       ip6_accept_rtadv = 1; /* XXX */
+       rv = rump_netconfig_ifup(ifname);
+       if (rv != 0)
+               goto out;
+       rv = (*rsso->so_send)(rsso, nam, NULL, m, NULL, 0, curlwp);

+       if (rv != 0)
+               goto out;
+       m = nam = NULL;
+       rv = 0;

So you fix these 4 lines to m = nam = NULL;

+out:
+       if (m)
+               m_freem(m);
+       if (nam)
+               m_freem(nam);
+       if (rsso)
+               soclose(rsso);
+       return rv;

Reply via email to