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

gnrc_netif: provide helper functions for NETOPT_IPV6_(ADDR|GROUP) #8253

Merged
merged 2 commits into from
Dec 18, 2017
Merged

gnrc_netif: provide helper functions for NETOPT_IPV6_(ADDR|GROUP) #8253

merged 2 commits into from
Dec 18, 2017

Conversation

miri64
Copy link
Member

@miri64 miri64 commented Dec 13, 2017 •
edited
Loading

Contribution description

Since especially the setter for NETOPT_IPV6_ADDR and the getter for
both NETOPT_IPV6_ADDR and NETOPT_IPV6_GROUP isn't that obvious, I
decided to provided a helper function for those gnrc_netapi_get/_set
operations.

Issues/PRs references

Depends on #8252 (merged).

@miri64 miri64 added GNRC State: waiting for other PR State: The PR requires another PR to be merged first labels Dec 13, 2017
@miri64 miri64 force-pushed the gnrc_netif/enh/ipv6-helper branch 2 times, most recently from fbef268 to 7105bf0 Compare December 13, 2017 14:20
bergzand
bergzand requested changes Dec 18, 2017
View reviewed changes
Copy link
Member

@bergzand bergzand left a comment

Choose a reason for hiding this comment

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

Only some remarks on the comments, looks good otherwise.

sys/include/net/gnrc/netif.h
* @pre `addrs != NULL`
* @pre `max_len >= sizeof(ipv6_addr_t)`
*
* @param[in] netif The interface. May not be `NULL`.
Copy link
Member

Choose a reason for hiding this comment

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

Judging from the assertions, shouldn't this be "Must not be NULL?

Copy link
Member Author

Choose a reason for hiding this comment

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

Mhhh... In the past I always used "May"... Maybe I'm wrong, but AFAIK "must not" and "may not" are synonymous (meaning "you can not do that"; at least that is what my English-German dictionary is telling me), while their opposite "may" (it is optional to do that) and "must" (it is required to do that) are not. Maybe "may not" is a little bit weaker in the ears of a native speaker, but for consistency I would prefer to keep it this way or to change my older wordings first.

Copy link
Member Author

Choose a reason for hiding this comment

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

Just realized however, that I used "Must not" here as well ^^"

Copy link
Member

Choose a reason for hiding this comment

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

Grammatically speaking I'd say you're completely correct here (not going to burn my fingers on English grammar). The reason I stumbled over this is because I'm interpreting these musts and mays as rfc2119 :)

I think together with the documented preconditions "may" is also clear enough.

Copy link
Member Author

Choose a reason for hiding this comment

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

Mhhh, but RFC2119 only documents "Must", "Must not" and "May"... or do you mean because "may not" it isn't documented the precondition is unclear?

Sidenote: I'm not aware if the terminology of our doc MUST be according to RFC 2119 ;-).

Copy link
Member

Choose a reason for hiding this comment

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

Mhhh, but RFC2119 only documents "Must", "Must not" and "May"... or do you mean because "may not" it isn't documented the precondition is unclear?

To be honest, I never noticed that "May not" was missing. No, I rather think that with the documented @pres, it is quite clear that the netif argument is mandatory.

Sidenote: I'm not aware if the terminology of our doc MUST be according to RFC 2119 ;-).

Never said it should be according to that, only that I'm interpreting these things by default as RFC 2119. Whether that is correct is a whole different discussion. ;-)

Copy link
Member Author

Choose a reason for hiding this comment

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

True, that's why I repeat myself:

[…] for consistency I would prefer to keep it this way or to change my older wordings first.

(I prefer to do it in a separate PR afterwards)

sys/include/net/gnrc/netif.h
* @pre `addr != NULL`
* @pre `(pfx_len > 0) && (pfx_len <= 128)`
*
* @param[in] netif The interface. May not be `NULL`.
Copy link
Member

Choose a reason for hiding this comment

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

Same as above

sys/include/net/gnrc/netif.h
* @pre `netif != NULL`
* @pre `addr != NULL`
*
* @param[in] netif The interface. May not be `NULL`.
Copy link
Member

Choose a reason for hiding this comment

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

And another may that should be a must

sys/include/net/gnrc/netif.h
* @pre `addr != NULL`
*
* @param[in] netif The interface. May not be `NULL`.
* @param[in] addr The address to remove from @p netif. May not be `NULL`.
Copy link
Member

Choose a reason for hiding this comment

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

Same as above.

sys/include/net/gnrc/netif.h
* @pre `groups != NULL`
* @pre `max_len >= sizeof(ipv6_addr_t)`
*
* @param[in] netif The interface. May not be `NULL`.
Copy link
Member

Choose a reason for hiding this comment

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

And another

sys/include/net/gnrc/netif.h
* @pre `group != NULL`
*
* @param[in] netif The interface.
* @param[in] group The address of the multicast group to join on @p netif.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add comments here to clarify the restrictions as done with the ipv6_groups_get function

Copy link
Member Author

Choose a reason for hiding this comment

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

(done in the line below)

sys/include/net/gnrc/netif.h
* @pre `group != NULL`
*
* @param[in] netif The interface.
* @param[in] group The address of the multicast group to leave on @p netif.
Copy link
Member

Choose a reason for hiding this comment

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

And also here the clarifications maybe

Copy link
Member Author

Choose a reason for hiding this comment

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

(done in the line below)

@miri64
Copy link
Member Author

miri64 commented Dec 18, 2017

Addressed comments (used "may not" were I used "must not" before; if this is blocking, I can change all to "must not"…).

@miri64
gnrc_netif: pass up returned error of internal function
8a058d3 
If there is no space left on the interface, a user should be notified
about that when using `gnrc_netapi_set()`
@miri64 miri64 force-pushed the gnrc_netif/enh/ipv6-helper branch from 4075cef to b6da6a9 Compare December 18, 2017 16:01
@miri64 miri64 added Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed State: waiting for other PR State: The PR requires another PR to be merged first labels Dec 18, 2017
@miri64
Copy link
Member Author

miri64 commented Dec 18, 2017

Rebased to current master.

@bergzand
Copy link
Member

ACK, please squash

@miri64 miri64 force-pushed the gnrc_netif/enh/ipv6-helper branch from b6da6a9 to 9681e5d Compare December 18, 2017 16:04
@miri64
Copy link
Member Author

miri64 commented Dec 18, 2017

Squashed

@miri64
gnrc_netif: provide helper functions for NETOPT_IPV6_(ADDR|GROUP)
16374d0 
Since especially the setter for `NETOPT_IPV6_ADDR` and the getter for
both `NETOPT_IPV6_ADDR` and `NETOPT_IPV6_GROUP` isn't that obvious, I
decided to provided a helper function for those `gnrc_netapi_get/_set`
operations.
@miri64 miri64 force-pushed the gnrc_netif/enh/ipv6-helper branch from 9681e5d to 16374d0 Compare December 18, 2017 16:07
@miri64
Copy link
Member Author

miri64 commented Dec 18, 2017

Fixed Murdock issues and squashed immediately (net/ipv6/addr.h include was missing in net/gnrc/netif.h).

@bergzand bergzand merged commit 21978b1 into RIOT-OS:master Dec 18, 2017
@miri64 miri64 deleted the gnrc_netif/enh/ipv6-helper branch December 18, 2017 16:18
@miri64 miri64 mentioned this pull request Dec 19, 2017
Merged
@aabadie aabadie added this to the Release 2018.01 milestone Jan 18, 2018
@miri64 miri64 added the Area: network Area: Networking label Sep 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bergzand bergzand bergzand approved these changes

Assignees

@bergzand bergzand

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 Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care.
Projects
None yet
Milestone
Release 2018.01
Development

Successfully merging this pull request may close these issues.
3 participants
@miri64 @bergzand @aabadie