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

IPv6: implement source address candidate selection #3561

Merged
merged 3 commits into from
Aug 17, 2015
Merged

IPv6: implement source address candidate selection #3561

merged 3 commits into from
Aug 17, 2015

Conversation

OlegHahm
Copy link
Member

@OlegHahm OlegHahm commented Aug 5, 2015

@OlegHahm OlegHahm added Area: network Area: Networking State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet NSTF labels Aug 5, 2015
@OlegHahm
Copy link
Member Author

OlegHahm commented Aug 5, 2015

Currently only the candidate address selection algorithm is implemented. The rest follows later today.

cgundogan
cgundogan reviewed Aug 5, 2015
View reviewed changes
sys/net/network_layer/ng_ipv6/netif/ng_ipv6_netif.c
uint8_t *candidate_set)
{
for (int i = 0; i < NG_IPV6_NETIF_ADDR_NUMOF; i++) {
ng_ipv6_addr_t *iter = &(iface->addrs[i].addr);
Copy link
Member

Choose a reason for hiding this comment

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

you might need to utilize the mutex of ng_ipv6_netif_t, before accessing any further members of this struct

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, see l514

Copy link
Member

Choose a reason for hiding this comment

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

nvm, you are using the mutex in L514

@OlegHahm OlegHahm added the State: waiting for other PR State: The PR requires another PR to be merged first label Aug 5, 2015
@OlegHahm
Copy link
Member Author

OlegHahm commented Aug 5, 2015

Depends on #3573.

@OlegHahm OlegHahm removed the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label Aug 6, 2015
@OlegHahm
Copy link
Member Author

OlegHahm commented Aug 6, 2015

Also depends on #3575 and #3577. Otherwise this is ready for review and testing.

@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 Aug 6, 2015
@miri64
Copy link
Member

miri64 commented Aug 7, 2015

Just wondering: are anycast addresses excluded (or rather should they be) in this algorithm too?

@OlegHahm
Copy link
Member Author

OlegHahm commented Aug 7, 2015

Not sure. First their are not explicitly mentioned - in fact I'm not even sure if comparison to anything else apart from link-local, site-local, and global unicast is described in the RFC, but since I have to compare the other types anyway to determine a global unicast address and it seems sensible to me, I did it this way. Second, is their a way to distinguish anycast addresses? A very brief look haven revealed anything, but maybe I gave up to quickly.

@miri64
Copy link
Member

miri64 commented Aug 7, 2015

Not sure. First their are not explicitly mentioned - in fact I'm not even sure if comparison to anything else apart from link-local, site-local, and global unicast is described in the RFC, but since I have to compare the other types anyway to determine a global unicast address and it seems sensible to me, I did it this way.

I'm not sure if anycast addresses are allowed to be source addresses… Too lazy to find a source right now, but they are definitely not global unicast addresses, as the name implies ;-).

Second, is their a way to distinguish anycast addresses? A very brief look haven revealed anything, but maybe I gave up to quickly.

Not syntactically, no. RFC 4291 states they should be marked as such on the interface. In our implementation this is achieved by the NG_IPV6_NETIF_ADDR_FLAGS_NON_UNICAST flag. You can use the ng_ipv6_netif_addr_is_non_unicast() function to check this for an address that is already assigned to the interface (be careful to use the right pointer) and the ng_ipv6_addr_is_multicast() function to exclude multicast addresses from the result set (when you test for global unicast addresses you already excluded those).

@OlegHahm
Copy link
Member Author

OlegHahm commented Aug 7, 2015

I'm not sure if anycast addresses are allowed to be source addresses…

RFC6724 says:
" Removed restriction against anycast addresses in the candidate set of source addresses, since the restriction against using IPv6 anycast addresses as source addresses was removed in Section 2.6 of RFC 4291 [RFC4291]."

Too lazy to find a source right now, but they are definitely not global unicast addresses, as the name implies ;-).

I know.

@OlegHahm
Copy link
Member Author

OlegHahm commented Aug 7, 2015

Rebased on master. Still depending on #3573.

@OlegHahm
Copy link
Member Author

OlegHahm commented Aug 7, 2015

About the anycast addresses: Apparently they are not forbidden as source address, but also they are not explicitly mentioned as being preferred in any rule. From a logical point of view, it doesn't seem sensible to prefer an anycast address as source address if the destination address is anycast. What do you think?

@miri64
Copy link
Member

miri64 commented Aug 7, 2015

From a logical point of view, it doesn't seem sensible to prefer an anycast address as source address if the destination address is anycast.

why has the destination address anything to do with that (and by the way I don't understand your logical conclusion that src should be preferred as NOT being anycast when the dst IS anycast)

@OlegHahm
Copy link
Member Author

OlegHahm commented Aug 7, 2015

why has the destination address anything to do with that

That's basically the only criteria for choosing the source address.

(and by the way I don't understand your logical conclusion that src should be preferred as NOT being anycast when the dst IS anycast)

That's not what I wrote. I say it doesn't seem to make sense to prefer the anycast address as source.

@OlegHahm
Copy link
Member Author

OlegHahm commented Aug 7, 2015

Pushed some code size optimizations. Not longer dependent on #3573.

@OlegHahm OlegHahm removed the State: waiting for other PR State: The PR requires another PR to be merged first label Aug 7, 2015
@OlegHahm OlegHahm mentioned this pull request Aug 7, 2015
Closed
@OlegHahm OlegHahm added the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label Aug 7, 2015
@OlegHahm
Copy link
Member Author

OlegHahm commented Aug 7, 2015

Reading the RFC again I figured out that rule 2 isn't implemented properly.

@OlegHahm
Copy link
Member Author

OlegHahm commented Aug 8, 2015

Hm, all the ways I tried so far are way too big. Hang on!

@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 Aug 12, 2015
@OlegHahm
Copy link
Member Author

addressed comments

@OlegHahm
Copy link
Member Author

fixed a comment, that was a leftover from a previous version

miri64
miri64 reviewed Aug 17, 2015
View reviewed changes
sys/net/network_layer/ng_ipv6/netif/ng_ipv6_netif.c
bool src_search)
/**
* @brief selects potential source address candidates
* see http://tools.ietf.org/html/rfc6724#section-4
Copy link
Member

Choose a reason for hiding this comment

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

use @see tag + <a href="…">RFC 6724, section 4</a>

Copy link
Member Author

Choose a reason for hiding this comment

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

It's a C file, but sure, I can do.

@OlegHahm
Copy link
Member Author

Hopefully addressed everything.

@miri64
Copy link
Member

miri64 commented Aug 17, 2015

Works! Let me have a last look at the code, then I'll ACK.

@miri64
Copy link
Member

miri64 commented Aug 17, 2015

ACK, please squash.

@OlegHahm OlegHahm added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable labels Aug 17, 2015
@OlegHahm
Copy link
Member Author

squashed

@OlegHahm
unittests: fixed ipv6_netif tests
65e2acb 
Anycasts are not explicitly forbidden according to RFC6724, Appendix B.
@OlegHahm
Copy link
Member Author

Fixed missing NG_IPV6_ADDR... ocurrences in unittests and squashed immediately.

OlegHahm added a commit that referenced this pull request Aug 17, 2015
@OlegHahm
Merge pull request #3561 from OlegHahm/ipv6_source_candidate_set
b68213a 
IPv6: implement source address candidate selection
@OlegHahm OlegHahm merged commit b68213a into RIOT-OS:master Aug 17, 2015
@OlegHahm OlegHahm deleted the ipv6_source_candidate_set branch August 17, 2015 23:17
@OlegHahm OlegHahm mentioned this pull request Aug 17, 2015
Closed
@miri64 miri64 mentioned this pull request Oct 9, 2019
Closed
miri64 added a commit to miri64/RIOT that referenced this pull request Jan 10, 2020
@miri64
gnrc_netif: remove misleading comment
d6914e8 
The comment exists since the introduction of the [original
implementation], but its meaning is unclear and misleading, as the code
doesn't do anything with link-local.

[original implementation]: RIOT-OS#3561
miri64 added a commit to miri64/RIOT that referenced this pull request Jan 14, 2020
@miri64
gnrc_netif: remove misleading comment
85737a0 
The comment exists since the introduction of the [original
implementation], but its meaning is unclear and misleading, as the code
doesn't do anything with link-local.

[original implementation]: RIOT-OS#3561
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@miri64 miri64

Labels
Area: network Area: Networking CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@OlegHahm @miri64 @cgundogan