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.

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

net/network_layer/fib: corrected handling of all 0 addresses #2783

Merged
merged 1 commit into from Jun 2, 2015

Conversation

BytesGalore
Copy link
Member

The FIB did not handled all 0 addresses correctly.
When a default GW IPv6 address :: has been added to the FIB it could not be removed by fib_remove_entry() or updated by fib_update_entry() since it has not been discovered as an exact matching address.

This PR adds a check if the searched entry is an all 0 address and adjusts the determination if we have an exact match, so all 0 addresses can be removed and updated correctly.

@cgundogan
Copy link
Member

@BytesGalore could you please rebase this one?

@BytesGalore
Copy link
Member Author

@cgundogan sure, done :)

@cgundogan
Copy link
Member

BTW, I noticed that you don't make any real use of prefix_len. Am I mistaken? It just sits at 0 until it gets assigned some value and then glides into nirvana.. Everywhere else you could've checked for 0 directly?

@BytesGalore
Copy link
Member Author

prefix_size? If yes, I use it to determine if the found prefix is the best LPM [1] :)

[1] https://github.com/BytesGalore/RIOT/blob/fib_support_all_zero_addr/sys/net/network_layer/fib/fib.c#L139

@cgundogan
Copy link
Member

@BytesGalore yes but, prefix_size is always 0 at this position. I cannot spot a location where it gets updated prior to the check?

@BytesGalore
Copy link
Member Author

ok, here [1] Its updated with match_size that is filled with the number of matching bytes until the first non-trailing 0 byte in universal_address_compare(...) :)

[1] https://github.com/BytesGalore/RIOT/blob/fib_support_all_zero_addr/sys/net/network_layer/fib/fib.c#L160

@cgundogan
Copy link
Member

And prior to this assignment you could exchange all occurences of prefix_size with a 0 constant? And after this assignment prefix_size isn't used anywhere else?

@cgundogan
Copy link
Member

I don't understand the function of prefix_size😦

@BytesGalore
Copy link
Member Author

ok, when we search a fitting entry we start to iterate all FIB entries [1]
If we find a matching one here [2], we see if the matching entry is a full match or if have found a prefix [3].
If its a prefix only we have to look if we can find a better one.
So we remember the current found one [4], store the found prefix_size and reset the matching size [5].
Then we loop again and see if we find a better prefix for the destination [2] (with the original match_size ).
In this subsequent iteration we can see if a found prefix is better if match_size > prefix_size [6]

I hope this helps :)

[1] https://github.com/BytesGalore/RIOT/blob/fib_support_all_zero_addr/sys/net/network_layer/fib/fib.c#L112
[2] https://github.com/BytesGalore/RIOT/blob/fib_support_all_zero_addr/sys/net/network_layer/fib/fib.c#L141
[3] https://github.com/BytesGalore/RIOT/blob/fib_support_all_zero_addr/sys/net/network_layer/fib/fib.c#L144
[4] https://github.com/BytesGalore/RIOT/blob/fib_support_all_zero_addr/sys/net/network_layer/fib/fib.c#L154
[5] https://github.com/BytesGalore/RIOT/blob/fib_support_all_zero_addr/sys/net/network_layer/fib/fib.c#L161
[6] https://github.com/BytesGalore/RIOT/blob/fib_support_all_zero_addr/sys/net/network_layer/fib/fib.c#L153

@cgundogan
Copy link
Member

ah the there is an outer loop! Excuse me, Sir! this changes everything (:

@cgundogan
Copy link
Member

I tested and can confirm: this works for the :: ip address => ACK. Merge when Travis builds and shows green (somewhen in the far future..)

@BytesGalore
Copy link
Member Author

nice, thx for testing

@cgundogan
Copy link
Member

Travis, why you no build..
@authmillenon Do you even ACK?

With your consent, I want to get this merged as soon as travis has a smiling face.

@cgundogan
Copy link
Member

I am not sure if this is related to this bug, but I seem to cannot add the default route to a fib table which has >= 1 entries. On a fib table with zero entries, adding the default route is a success.

@cgundogan
Copy link
Member

My ACK still holds. I am not sure if the strange behavior reported in my last comment is related to this PR. If yes, a follow-up PR can fix it. GO

cgundogan added a commit that referenced this pull request Jun 2, 2015
net/network_layer/fib: corrected handling of all 0 addresses
@cgundogan cgundogan merged commit bdc12b0 into RIOT-OS:master Jun 2, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: network Area: Networking Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants