no need for @ref here?

Don't know why this reference is there anyway... #7456 moves that to gnrc_netif2_t and _nib_iface_t is internal and thus not in the generated doc...

Gets

Handles

here and same for following funcs

the function only handles icmpv6. should we put that info somewhere into the name of the func?

I think the function name is sufficiently long. If someone does not check by here that the neighbor information base only concerns NDP (and thus ICMPv6) they should not meddle with this function anyway.

boo :P

Doccheck said its broken :P

Could you add a TODO to the doxygen? It should be obvious somewhere that multihop DAD is not working currently.

Will do in sys/include/net/gnrc/ipv6/nib.h

4 levels of indentation .. if it's no hassle, could you try to remove some nestedness here?

riginal

why not assert this? Is it possible that – by design – NULL is passed to this function as aro? If not by design, then this is an unexpected behaviour and should be handled on the caller side.

Yes, if the message does not contain an ARO this function is called regardless (to keep 6LR-mixins minimal), to check that fact.

Hm, strange. This file is called _nib-arsm.c. Why shouldn't GNRC_IPV6_NIB_CONF_ARSM be defined on this code path?

Because sending Neighbor Solicitations is (originally) related to address resolution, but also required for other stuff (e.g. 6Lo address registration and SLAAC). Welcome to NDP hell ;-).

(the ARSM-only part starts in l172)

SL2AO

SL2AO

I have to admit, I don't like this function. It's too complex and I spent a lot of time to understand what's going on here – and I am quite certain I will spent the same amount of time a year later from now. Please try to remove the nested if - else hell. The Clean Code approach would be to split this function into several smaller (inlined) ones with well-documented and obvious names. I am not sure whether that's the solution for us, but the current implementation is very unreadable and IMO unmaintainable in the future.

I don't like it either, but that's exactly how the RFC reads. Welcome to NDP hell ;-)

Will try to fix it though.

TL2AO

There's no need to re-seed the PRNG iff the start seed is distinct on all nodes. And the latter is what we should aim for.

And btw, you break here the PRNG for all applications that use this random interface with the same PRNG state.

There's no need to re-seed the PRNG iff the start seed is distinct on all nodes. And the latter is what we should aim for.

But it is not the case now. Also: this is a very standard approach to get a distinct random number.

And btw, you break here the PRNG for all applications that use this random interface with the same PRNG state.

If re-seeding breaks your PNRG then your PNRG is broken

Sorry, let me rephrase:

You are using the system/global PRNG state here. The moment you re-seed, the PRNG for all applications that make use of the system/global state do not produce deterministic results anymore. A PRNG is supposed to yield deterministic results.

Oh and btw: random_init() is not thread-safe. So please let the seeding be performed at auto_init() once, like it's the case now, or provide a separate PRNG state for NIB that can be re-seeded without global effects.

or provide a separate PRNG state for NIB that can be re-seeded without global effects.

and even then I doubt that re-seeding provides "better randomization".

and even then I doubt that re-seeding provides "better randomization".

Why are you doubting that? Currently the PNRG is always seeded with 0, there are plans to use the luid as seed (making it different but deterministic for different nodes). This reseed uses the reception time of the first received solicited NA from all neighbors, which is always naturally random, due to the different air times of both NS and NA, so it is by definition "better randomization" than using the PNRG with a deterministic seed.

What about the other points?

1. random_init() is not thread-safe
2. you modify the global PRNG state which is potentially used by more thread than just the network stack

I propose that you factor out this re-seeding procedure into another PR and we continue the discussion there, so that we can continue here?

Fine...

Done

here

I'm not fixing a note to myself :P

I was just pointing it out .. you can decide what you do with it (:

here

I'm not fixing a note to myself :P

question: should we put this into doxygen to make this TODO more obviouvs?

These are implemented in #7456 and #7479, so why?

Since when do we use these hidden gotos? Frankly said, I do not like this idea. It 1) gives another level of indentation, 2) is an unconditional jump the same way as goto (so the same disadvantages apply) and 3) bloats the code by two more lines. I am sure you can re-arrange the code to not use this hidden goto and it still results in a more readable version (:

Since when do we use these hidden gotos? Frankly said, I do not like this idea.

Since @gebart proposed them as an alternative to "finally-gotos".

1. gives another level of indentation,

Since when is there this hatred for levels of indentations? We are not the Linux kernel (where this Coding Convention lead to very weird code btw).

I am sure you can re-arrange the code to not use this hidden goto and it still results in a more readable version (:

I find it quite readable.

mutex_lock(&_nib_mutex);

if (!ipv6_addr_is_link_local(dst)) {
    /* TODO: Off-link next hop determination */
    mutex_unlock(&_nib_mutex);
    return -EHOSTUNREACH;
}

if ((iface == KERNEL_PID_UNDEF) ||
    !_resolve_addr(dst, iface, pkt, nce, _nib_onl_get(dst, iface))) {
    mutex_unlock(&_nib_mutex);
    return -EHOSTUNREACH;
}

mutex_unlock(&_nib_mutex);
return 0;

Isn't this so much simpler to read without any level of nestedness?!

This adds 2 more function calls (mutex_unlock()), 6 in #7479 (mutex_unlock() + gnrc_netif2_release()), and even a more simpler version [edit]with only one additional mutex_unlock()[/edit]

    int res = 0;

    mutex_lock(&_nib_mutex);
    if (!ipv6_addr_is_link_local(dst)) {    /* TODO: Prefix-based on-link determination */
        /* TODO: Off-link next hop determination */
        mutex_unlock(&_nib_mutex);
        return -EHOSTUNREACH;
    }
    if ((iface == KERNEL_PID_UNDEF) ||
        !_resolve_addr(dst, iface, pkt, nce,
                       _nib_onl_get(dst, iface))) {
        res = -EHOSTUNREACH;
    }
    mutex_unlock(&_nib_mutex);
    return res;

Adds 24 byte of ROM more on Cortex-M0.

So many TODOS that could end up in a nicely documented fashion => doxygen (:

I'm not sure if putting TODOs in doxygen is productive here...

where is it discarded?

        else {
            reply_flags |= NDP_NBR_ADV_FLAGS_O;
        }
    }
    else {
        reply_flags |= NDP_NBR_ADV_FLAGS_O;
    }

you were able to remove such code duplication in another PR, can you also apply this here, please?

There it was at the end of a function, so I was just able to early exit. This is not possible here.

I could use a "hidden goto" instead ;-)