sys/net/fib: added function to request a set of destination addresses

[BytesGalore]

This PR introduces an access function to the FIB, that collects all destination addresses matching specific prefix.
(This is meant as a base for discussion)

Rationale:
Using the FIB with RPL requires in storing MOP that all destination addresses reachable from a node, which are constructed by receiving DIOs and all target options from all received DAOs recursively, are put in the DAO messages disseminated by the node.
The former FIB did not provided access for a set of destination addresses matching a specific prefix.

[cgundogan]

Could you move the check dst_set != NULL outside of the loop?

[BytesGalore]

Basically yes, but the it would require a construct like:

if( dst_set != NULL ){
 for(...) ... // like above without the check
} else {
 for(...) ... // just counting the entries
}

[cgundogan]

I see. Actually I thought that this function should return an error when no dst_set was specified. Anyhow, can you extend the doc that this function still counts and returns the number of all matching entries even when they are not copied to dst_set?

And add parens to the condition, please (:

[BytesGalore]

Added parens and wrote a line that the matching count is being determined.

The idea to allow a dst_set == NULL and continue, is that if you're interested at first in the current number of matching entries, to check if your out buffer can hold them, you probably don't want to have any copy operations.

[cgundogan]

could you also add the fact, that the buffer is allowed to be null? Something like:

If the out buffer is insufficient low or null, ...

[BytesGalore]

sure, sorry for the delay.

[cgundogan]

besides my comment this PR looks good and needs squashing

[cgundogan]

does anyone else wants to participate in this discussion (because it's marked as RFC)? Otherwise, my ACK holds and my intention is to merge this ASAP

[BytesGalore]

squashed and rebased.

[cgundogan]

I guess I am satisfied with this PR and I am in favor of dropping the RFC label here and opt for a quick merge, since we need this functionality for #2765 . What do you say @BytesGalore @OlegHahm ?

[BytesGalore]

@cgundogan sure, removed the RFC label

[BytesGalore]

in a followup PR we could make this feature optional/exclusive when RPL is loaded

[emmanuelsearch]

Do we need to think about a rationale such that their are no flag collisions between different protocols that write into the FIB? Like a prefix based on protocol ID, and then the rest of the bits are managed per protocol?

[BytesGalore]

@emmanuelsearch definitely, I think an enum with define protected members will do, similar to [1]

[1] https://github.com/RIOT-OS/RIOT/blob/master/sys/include/net/ng_nettype.h#L89

[BytesGalore]

I don't expect that we will have like 100 routing-protocols available and using more than 3 at the same time on a node

[cgundogan]

@BytesGalore are global_flags and next_hop_flags only used for routing protocol distinction? What other uses for them are out there? Introducing a mechanism for collision-free type definitions mentioned by @emmanuelsearch is favorable. Personally, I have no strong opinion of it being an enum or somehow encoding this information by using specific prefixes. I would argue however that this should be done in a separate PR and most preferably prior to this one.

regardless of the fact that we will not have more than max 3 routing protocols running at the same time, an enum or well-chosen ifdefs would make access to these flags more readable than some magic hex numbers?

[BytesGalore]

@cgundogan the _flags are used also to identify the address type, e.g. NG_NETTYPE_IPV6.
Its beneficial to know what type of address hides behind the address bytes, and is necessary when we provide using multiple Protocols with different address types.

But I think you're both are right. A collision free mechanism based on protocol IDs and additional management bits is the right way to do it and this should be a done/merged prior to this PR.

[cgundogan]

@BytesGalore do you have a concept in mind for such a mechanism?

[BytesGalore]

@cgundogan after rethinking a bit I think using flags/protocol IDs for that purpose is not necessary.
A routing protocol is responsible for a specific prefix. All destination addresses are indirectly "assigned" to a protocol by the prefix.
So asking the FIB for a set of destination addresses based on a given prefix seems more reasonable for me here.

[BytesGalore]

The prototype would change to:

int fib_get_destination_set(uint32_t* prefix, size_t prefix_length, fib_destination_set_entry_t *dst_set, size_t* dst_set_size);

@cgundogan what do you think, any cons on change it this way?

[cgundogan]

@BytesGalore I guess your suggestion is reasonable. Assigning the routing protocols to handle certain prefixes may be the right way. To stress a point: What happens if prefix ranges overlap? longest prefix matches?

[BytesGalore]

@cgundogan overlapping should MUST NOT happen since then both (or multiple) routing protocols would take responsibility/royalty over the intersected address range.
This way the overlapping entries would be repeatedly overwritten form multiple sources with distinct values, preventing a healthy topology for each routing protocol using that shared range.
In consequence such approach will eventually result in routing loops since no routing protocol knows the current state of the topology in the overlapping range.

[BytesGalore]

ok, changed the approach to use a prefix match instead of flags, and rebased.

[OlegHahm]

I think there's only one choice: add it to BOARD_INSUFFICIENT_RAM in tests/unittests/Makefile.

[cgundogan]

@OlegHahm isn't it the rom section, which is overflowing?

[OlegHahm]

Ah, yes, you're right. Which is actually weird.

[miri64]

The macro BOARD_INSUFFICIENT_RAM refers to both ROM and RAM, since it just triggers the test to ignore any linking errors...

[cgundogan]

Then we should opt to change this macro's name to be more verbose and semantically correct

[OlegHahm]

Maybe we should rename it to BOARD_INSUFFICIENT_MEMORY then.

[cgundogan]

Maybe we should rename it to BOARD_INSUFFICIENT_MEMORY then.

+1

[BytesGalore]

+1

[BytesGalore]

I added the samr board to BOARD_INSUFFICIENT_RAM

[cgundogan]

@BytesGalore please squash. Merge when travis is happy

[cgundogan]

@BytesGalore I think this PR also needs a rebase?

[BytesGalore]

rebased and squashed

[cgundogan]

@BytesGalore can you trigger travis?

[BytesGalore]

hmm seems that travis is currently not available :(

[cgundogan]

Why is travis not building this PR? It works for other PRs..

[cgundogan]

@BytesGalore can you again rebase to master so that #2783 is also included in this branch? This would simplify the rebase of my branch in #3050 (:

[cgundogan]

travis is green => GO