Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

net/network_layer/fib: added prefix consideration for RRP registration/signaling #2915

Merged
merged 1 commit into from
May 14, 2015
Merged

net/network_layer/fib: added prefix consideration for RRP registration/signaling #2915

merged 1 commit into from
May 14, 2015

Conversation

BytesGalore
Copy link
Member

This PR forces to provide a prefix on register Reactive Routing Protocol (RRP), which is used to determine if the RRP can find a route to a searched destination address.

Rationale:
The current FIB does not consider a prefix per RRP registration.
This way all registered RRPs are signalled to start a route discovery for a given destination address without a check if the RRP can handle the prefix for the given destination.

@BytesGalore BytesGalore added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: network Area: Networking Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. NSTF labels May 5, 2015
@BytesGalore BytesGalore added this to the Network Stack Task Force milestone May 5, 2015
Lotterleben
Lotterleben reviewed May 5, 2015
View reviewed changes
sys/net/network_layer/fib/fib.c
@@ -294,16 +299,19 @@ static int fib_signal_rrp(uint8_t *dst, size_t dst_size, uint32_t dst_flags)
msg.content.ptr = (void *)&content;

for (size_t i = 0; i < FIB_MAX_RRP; ++i) {
if (notify_rrp[i] != KERNEL_PID_UNDEF) {
if ((notify_rrp[i] != KERNEL_PID_UNDEF) && (prefix_rrp[i] != NULL)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this check really need to happen if you're forcing protocols to supply their prefix anyway? If I understand it correctly, the purpose of this line is to weed out the “empty” entries, so just checking for KERNEL_PID_UNDEF should suffice, shouldn't it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops true, I was a bit overcautious here.

@BytesGalore BytesGalore force-pushed the fib_register_rrp_with_prefix branch from 4fd96bc to 17ea02d Compare May 5, 2015 16:27
@BytesGalore
Copy link
Member Author

squashed, now its travis time.

@Lotterleben
Copy link
Member

Goooo Travis! ᕕ( ᐛ )ᕗ (σ・・)σ ♪┏(・o･)┛♪┗ ( ･o･)

@BytesGalore
Copy link
Member Author

@Lotterleben I forgot that I want to change the name of the function 😁 since it should be used by any routing protocol regardless if its reactive or proactive. will be:

int fib_register_rp(uint8_t *prefix, size_t prefix_size);

Any complaints renaming it here? (I will do a commit but we can remove it here)

@BytesGalore BytesGalore force-pushed the fib_register_rrp_with_prefix branch from bd083d8 to eda6bc9 Compare May 6, 2015 05:45
@OlegHahm OlegHahm added the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label May 6, 2015
@Lotterleben
Copy link
Member

@BytesGalore nope, that name is fine by me :)

@Lotterleben
Copy link
Member

What about the (prefix_size == 0) check we talked abut earlier, though? :)

@BytesGalore
Copy link
Member Author

@Lotterleben a perfix_size of 0 would result in an 0 size universal_address_t address stored as prefix for the routing protocol.
I think this should be prevented :)

@BytesGalore BytesGalore force-pushed the fib_register_rrp_with_prefix branch from eda6bc9 to 2a83b56 Compare May 6, 2015 09:25
@BytesGalore
Copy link
Member Author

squashed.

@BytesGalore BytesGalore removed the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label May 6, 2015
@Lotterleben
Copy link
Member

@BytesGalore argh sorry, I got confused. Of course.

@Lotterleben
Copy link
Member

Then, ACK when travis is happy

@Lotterleben
Copy link
Member

Kicked Travis... Seems like there was some interference with github's downtime.

@BytesGalore BytesGalore force-pushed the fib_register_rrp_with_prefix branch from 2a83b56 to 83abf1d Compare May 11, 2015 07:26
@Lotterleben
Copy link
Member

I'd love to merge this, just don't know what seems to be the problem according to Travis.. :(

@OlegHahm
Copy link
Member

Trying to build the unittest for any platform results in

"make" -C /home/oleg/git/RIOT/sys/net/network_layer/fib
/home/oleg/git/RIOT/sys/net/network_layer/fib/fib.c: In function 'fib_signal_rp'
/home/oleg/git/RIOT/sys/net/network_layer/fib/fib.c:294:5: error: unknown type name 'rrp_address_msg_t'
     rrp_address_msg_t content;
     ^

@BytesGalore BytesGalore force-pushed the fib_register_rrp_with_prefix branch from 0ce52f6 to bb4b687 Compare May 12, 2015 13:17
@BytesGalore BytesGalore added the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label May 12, 2015
@BytesGalore
Copy link
Member Author

@OlegHahm thx, I think I forced pushed the broken line :( sorry.

@BytesGalore
Copy link
Member Author

As I introduced a new msg type here I wonder if it would make sense to collect all configurable types/options/defines for the FIB in one specific header to easily be able to configure all aspects.

What do you think @Lotterleben, @OlegHahm, @cgundogan ?

@OlegHahm
Copy link
Member

There seems to be something else broken:

/home/oleg/git/RIOT/sys/net/network_layer/fib/fib.c: In function 'fib_signal_rp'
/home/oleg/git/RIOT/sys/net/network_layer/fib/fib.c:300:16: error: 'FIB_MSG_RRP_SIGNAL' undeclared (first use in this function)
     msg.type = FIB_MSG_RRP_SIGNAL;
                ^
/home/oleg/git/RIOT/sys/net/network_layer/fib/fib.c:300:16: note: each undeclared identifier is reported only once for each function it appears in

@OlegHahm
Copy link
Member

As I introduced a new msg type here I wonder if it would make sense to collect all configurable types/options/defines for the FIB in one specific header to easily be able to configure all aspects.

Sounds like a good idea to me.

@Lotterleben
Copy link
Member

+1 regarding the header.

@BytesGalore
Copy link
Member Author

ok, beside the need for squashing travis seems to be quite happy

@OlegHahm
Copy link
Member

Let's squash then.

@BytesGalore BytesGalore force-pushed the fib_register_rrp_with_prefix branch from 60a79c5 to 85bf1c2 Compare May 13, 2015 18:08
@BytesGalore BytesGalore removed the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label May 13, 2015
@BytesGalore
Copy link
Member Author

Ok, then I think I will do a fib_config.h or akin.
rebased and squashed.

@Lotterleben
Copy link
Member

Are you going to do open an new PR for the fib_config.h or should we wait for it before merging?

@BytesGalore
Copy link
Member Author

@Lotterleben I will open a new PR for the configuration header. It will be transparent for the user, so we don't need to wait to merge :) .

@Lotterleben
Copy link
Member

Well then let's go!

Lotterleben added a commit that referenced this pull request May 14, 2015
@Lotterleben
Merge pull request #2915 from BytesGalore/fib_register_rrp_with_prefix
6a0a2e8 
net/network_layer/fib: added prefix consideration for RRP registration/signaling
@Lotterleben Lotterleben merged commit 6a0a2e8 into RIOT-OS:master May 14, 2015
@Lotterleben Lotterleben mentioned this pull request May 15, 2015
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@Lotterleben Lotterleben

Labels
Area: network Area: Networking Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Milestone
Network Stack Task Force
Development

Successfully merging this pull request may close these issues.
3 participants
@BytesGalore @Lotterleben @OlegHahm