Skip to content

ipv6_netif: add prefix list to interface#2723

Merged
miri64 merged 2 commits intoRIOT-OS:masterfrom
miri64:ipv6_netif/feat/prefix-list
Mar 29, 2015
Merged

ipv6_netif: add prefix list to interface#2723
miri64 merged 2 commits intoRIOT-OS:masterfrom
miri64:ipv6_netif/feat/prefix-list

Conversation

@miri64
Copy link
Copy Markdown
Member

@miri64 miri64 commented Mar 26, 2015

This adds the prefix list, needed for Neighbor Discovery to ipv6_netif by adding a prefix_len field to its address list.

@miri64 miri64 added Area: network Area: Networking Type: new feature The issue requests / The PR implemements a new feature for RIOT Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer NSTF Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. labels Mar 26, 2015
@miri64 miri64 added this to the Network Stack Task Force milestone Mar 26, 2015
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

sure you want the prefix to be either 0 or >128? -> s/not//

@haukepetersen
Copy link
Copy Markdown
Contributor

looks sane to me, ACK if the comments above are addressed.

@miri64 miri64 force-pushed the ipv6_netif/feat/prefix-list branch from cc89aa7 to d053fd8 Compare March 26, 2015 11:27
@miri64
Copy link
Copy Markdown
Member Author

miri64 commented Mar 26, 2015

Addressed comments and rebased.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

prefix_len < 1 is only checking the case for prefix_len being 0 (because it's defined as uint8_t and never becomes negative). You could replace this by (!prefix_len) || (prefix_len > 128).
But I would prefer the (maybe more optimized) alternative: ((unsigned) x - 1 > 127). This will result in true for x < 1 and x > 128.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

always choose readability before optimizations. The compiler will probably choose the fastest evaluation of the condition anyway.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

IMHO, there is nothing wrong in helping the compiler to achieve a good level of optimization (: Nevertheless, since we are dealing with a wide range of platforms and compilers, it might make sense to manually apply some basic optimizations (like this one for range checking)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done. (I don't like writing !prefix_len instead of prefix_len == 0 though)

@miri64 miri64 force-pushed the ipv6_netif/feat/prefix-list branch from 679f066 to 7af5420 Compare March 26, 2015 12:11
@miri64
Copy link
Copy Markdown
Member Author

miri64 commented Mar 26, 2015

And rebased once again.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this has to be >, not <.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Args… right. Corrected.

@miri64 miri64 force-pushed the ipv6_netif/feat/prefix-list branch 2 times, most recently from 627c987 to 475cbc5 Compare March 27, 2015 19:34
@miri64 miri64 force-pushed the ipv6_netif/feat/prefix-list branch from 475cbc5 to d908421 Compare March 27, 2015 19:40
@miri64
Copy link
Copy Markdown
Member Author

miri64 commented Mar 27, 2015

Had a little error: multicast-addresses do not have prefixes in the sense that unicast/anycast addresses have: I set the prefix length for all multicast addresses to 128.

@miri64
Copy link
Copy Markdown
Member Author

miri64 commented Mar 29, 2015

And go

miri64 added a commit that referenced this pull request Mar 29, 2015
@miri64 miri64 merged commit 7273d5c into RIOT-OS:master Mar 29, 2015
@miri64 miri64 deleted the ipv6_netif/feat/prefix-list branch March 29, 2015 22:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: network Area: Networking Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. Type: new feature The issue requests / The PR implemements a new feature for RIOT

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants