Skip to content
New issue

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

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

Already on GitHub? # to your account

net/gcoap: Require interface for link local address in shell example #8223

Merged
merged 1 commit into from
Dec 10, 2017

Conversation

kb2ma
Copy link
Member

@kb2ma kb2ma commented Dec 7, 2017

Networking infrastructure updates require that a message to a link local address specifies the interface to use, as discussed in #8199. This PR accepts an interface ID in the gcoap shell example, and requires it for a link local address. See the example below.

coap get fe80::d8b8:65ff:feee:121b%6 5683 /.well-known/core

remote.netif = SOCK_ADDR_ANY_NETIF;

/* parse for interface */
int iface = ipv6_addr_split_iface(addr_str);
Copy link
Member

Choose a reason for hiding this comment

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

The ping6 command allows this to be ignored if only one interface exist (using #if GNRC_NETIF_NUMOF == 1U). In that case the first (and only) interface is selected.

Copy link
Member Author

@kb2ma kb2ma Dec 8, 2017

Choose a reason for hiding this comment

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

I understand your request. Thinking of the issue in #8130 with dependence on GNRC, is it a problem that I'll need to use GNRC functions to solve the issue here? Or is that the purpose of #6413 (stack-independent netif), and we'll need to update the interface lookup here when that PR is merged?

Copy link
Member

Choose a reason for hiding this comment

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

Or is that the purpose of #6413 (stack-independent netif), and we'll need to update the interface lookup here when that PR is merged?

Yes, but also: this application at the moment is also dependent on GNRC (it pulls in GNRC dependencies), so I think it is okay for now.

@miri64 miri64 added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Area: network Area: Networking labels Dec 7, 2017
Copy link
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

Two minor nit-picks. Otherwise ready to merge

int iface = ipv6_addr_split_iface(addr_str);
if (iface == -1) {
if (gnrc_netif_numof() == 1) {
remote.netif = (uint16_t)gnrc_netif_iter(NULL)->pid;
Copy link
Member

Choose a reason for hiding this comment

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

Maybe leave a comment why this is okay without checking.


/* parse destination address */
if (ipv6_addr_from_str(&addr, addr_str) == NULL) {
puts("gcoap_cli: unable to parse destination address");
return 0;
}
if (remote.netif == SOCK_ADDR_ANY_NETIF && ipv6_addr_is_link_local(&addr)) {
Copy link
Member

Choose a reason for hiding this comment

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

Parenthesis around first check

@@ -149,6 +149,7 @@ static size_t _send(uint8_t *buf, size_t len, char *addr_str, char *port_str)
int iface = ipv6_addr_split_iface(addr_str);
if (iface == -1) {
if (gnrc_netif_numof() == 1) {
// assign the single interface found in gnrc_netif_numof()
Copy link
Member

Choose a reason for hiding this comment

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

Please use C-style comments /* ... */ ;-)

@kb2ma
Copy link
Member Author

kb2ma commented Dec 10, 2017

That was embarassing. =:-0

Copy link
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

ACK, please squash.

Copy link
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

Needs more ☕

@kb2ma kb2ma force-pushed the gcoap/cli_link_local branch from a9a42a1 to d36c07c Compare December 10, 2017 13:04
@kb2ma
Copy link
Member Author

kb2ma commented Dec 10, 2017

Squashed and ready. Thanks for review.

@miri64 miri64 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Dec 10, 2017
Copy link
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

One last thing: please make teh commit message shorter than 50 characters (or 72 at the pare minimum)

@kb2ma kb2ma force-pushed the gcoap/cli_link_local branch from d36c07c to a1efa03 Compare December 10, 2017 14:54
@miri64 miri64 merged commit b8eb857 into RIOT-OS:master Dec 10, 2017
@aabadie aabadie added this to the Release 2018.01 milestone Jan 18, 2018
@kb2ma kb2ma deleted the gcoap/cli_link_local branch February 4, 2019 11:22
# for free to join this conversation on GitHub. Already have an account? # to comment
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 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.

3 participants