On Wed, Dec 21, 2016 at 01:52:45PM +0100, Martin Pieuchot wrote:
> ok?
OK bluhm@, but comments inline
> @@ -430,15 +430,16 @@ nd6_llinfo_timer(void *arg)
> void
> nd6_timer_work(void *null)
> {
> - int s;
> struct nd_defrouter *dr, *ndr;
> struct nd_prefix *pr, *npr;
> struct in6_ifaddr *ia6, *nia6;
> + int s;
>
> - s = splsoftnet();
> timeout_set(&nd6_timer_ch, nd6_timer, NULL);
> timeout_add_sec(&nd6_timer_ch, nd6_prune);
>
> + NET_LOCK(s);
> +
Moving timeout_set() out of the splsoftnet() feels wrong. The whole
task construct could be replaced with a timeout_set_proc(), I will
make a diff.
> @@ -1120,7 +1121,6 @@ nd6_ioctl(u_long cmd, caddr_t data, stru
> struct in6_nbrinfo *nbi = (struct in6_nbrinfo *)data;
> struct rtentry *rt;
> int error = 0;
> - int s;
>
Could you add an assert here when you remove the splsoftnet(). This
makes further review easier and we see bugs during runtime.
> @@ -1478,12 +1471,14 @@ fail:
> void
> nd6_slowtimo(void *ignored_arg)
> {
> - int s = splsoftnet();
> struct nd_ifinfo *nd6if;
> struct ifnet *ifp;
> + int s;
>
> timeout_set(&nd6_slowtimo_ch, nd6_slowtimo, NULL);
> timeout_add_sec(&nd6_slowtimo_ch, ND6_SLOWTIMER_INTERVAL);
> +
> + NET_LOCK(s);
Again timeout_set() without lock feels wrong. It has been done in
nd6_init() already. And there it is a timeout_set_proc() now.
Remove the line.
> @@ -825,7 +825,6 @@ nd6_na_input(struct mbuf *m, int off, in
> */
> struct nd_defrouter *dr;
> struct in6_addr *in6;
> - int s;
Please add an assert.
> nd6_dad_starttimer(struct dadq *dp, int ticks)
> {
>
> - timeout_set(&dp->dad_timer_ch, nd6_dad_timer, dp->dad_ifa);
> + timeout_set_proc(&dp->dad_timer_ch, nd6_dad_timer, dp->dad_ifa);
> timeout_add(&dp->dad_timer_ch, ticks);
Are we sure that timeout_set() is not called when the timer is
already running? I will have a look. You diff does not make it
worse.
> @@ -202,7 +202,7 @@ nd6_rs_output(struct ifnet* ifp, struct
> struct nd_router_solicit *rs;
> struct ip6_moptions im6o;
> caddr_t mac;
> - int icmp6len, maxlen, s;
> + int icmp6len, maxlen;
Can you put an assert here?
> @@ -1592,9 +1592,8 @@ pfxlist_onlink_check(void)
> if ((pr->ndpr_stateflags & NDPRF_DETACHED) != 0 &&
> (pr->ndpr_stateflags & NDPRF_ONLINK) != 0) {
> if ((e = nd6_prefix_offlink(pr)) != 0) {
> - nd6log((LOG_ERR,
> - "pfxlist_onlink_check: failed to "
> - "make %s/%d offlink, errno=%d\n",
> + nd6log((LOG_ERR, "%s: failed to make %s/%d"
> + " offlink, errno=%d\n", __func__,
> inet_ntop(AF_INET6,
> &pr->ndpr_prefix.sin6_addr,
> addr, sizeof(addr)),
> @@ -1605,9 +1604,8 @@ pfxlist_onlink_check(void)
> (pr->ndpr_stateflags & NDPRF_ONLINK) == 0 &&
> pr->ndpr_raf_onlink) {
> if ((e = nd6_prefix_onlink(pr)) != 0) {
> - nd6log((LOG_ERR,
> - "pfxlist_onlink_check: failed to "
> - "make %s/%d offlink, errno=%d\n",
> + nd6log((LOG_ERR, "%s: failed to make %s/%d"
> + " offlink, errno=%d\n", __func__,
> inet_ntop(AF_INET6,
> &pr->ndpr_prefix.sin6_addr,
> addr, sizeof(addr)),
> @@ -2000,7 +1998,7 @@ in6_init_address_ltimes(struct nd_prefix
Plaese keep the style that the space is before the " at the end
of the line.
