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

sys/net/sock_util: don't require interface for link-local addr if there is only a single interface #17927

Closed
wants to merge 3 commits into from
Closed

sys/net/sock_util: don't require interface for link-local addr if there is only a single interface #17927

wants to merge 3 commits into from

Conversation

benpicco
Copy link
Contributor

@benpicco benpicco commented Apr 13, 2022 •
edited
Loading

Contribution description

When the sock_tl_str2ep() function is supplied with a link-local address but no interface, if there is only a single interface use that instead of not setting the interface.

This is to match the behavior of the ping command.

Testing procedure

tests/nanocoap_cli is a user of this API.

> url get coap://[fe80::90a7:a6ff:fe4b:2e32%5]/hello.txt     
url get coap://[fe80::90a7:a6ff:fe4b:2e32%5]/hello.txt
optpos: 0x8072da1
offset 000: Hello RIOT!

still works but now you can also do

> url get coap://[fe80::90a7:a6ff:fe4b:2e32]/hello.txt
url get coap://[fe80::90a7:a6ff:fe4b:2e32]/hello.txt
optpos: 0x8072da1
offset 000: Hello RIOT!

If there is only a single interface.

Issues/PRs references

@github-actions github-actions bot added Area: network Area: Networking Area: sys Area: System labels Apr 13, 2022
@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Apr 13, 2022
@benpicco benpicco requested review from HendrikVE and kfessel and removed request for kfessel April 13, 2022 10:46
@benpicco benpicco added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Apr 13, 2022
HendrikVE
HendrikVE suggested changes Apr 18, 2022
View reviewed changes
sys/net/sock/sock_util.c
int _get_default_netif(sock_udp_ep_t *ep_out)
{
netif_t *netif = netif_iter(NULL);
if (netif == NULL || netif_iter(netif) != NULL) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why checking again for netif_iter(netif) != NULL ? According to doc netif_iter returns NULL, if there is no interface after @p last, which would be already true for the first call of netif_iter.

Copy link
Contributor Author

@benpicco benpicco Apr 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

  • netif_iter(NULL) gives the first interface
  • netif_iter(netif_iter(NULL)) gives the second interface
  • and so on

This should only set the interface if there is a first, but not a second interface. (to mirror the behavior of netutils_get_ipv6())

sys/net/sock/sock_util.c Show resolved Hide resolved
@github-actions github-actions bot added the Area: tests Area: tests and testing framework label Apr 19, 2022
@benpicco benpicco force-pushed the sock_util-default_if branch from a208497 to 1971855 Compare April 19, 2022 20:33
HendrikVE
HendrikVE reviewed Apr 25, 2022
View reviewed changes
tests/netutils/main.c Outdated Show resolved Hide resolved
@benpicco benpicco force-pushed the sock_util-default_if branch from 1971855 to 4aca39c Compare April 29, 2022 20:28
benpicco
benpicco commented May 3, 2022
View reviewed changes
sys/net/sock/sock_util.c Outdated Show resolved Hide resolved
benpicco and others added 3 commits May 3, 2022 11:53
@benpicco
sys/net/sock_util: don't require interface if there is only a single one
a6c5eae 
When the sock_tl_str2ep() function is supplied with a link-local address
but no interface, if there is only a single interface use that instead of
not setting the interface.
@benpicco
net/netif: add netif_unregister()
87f0c6f
@benpicco
tests/netutils: add test with a single interface
cf3cbce
@benpicco benpicco force-pushed the sock_util-default_if branch from 46dd6b2 to cf3cbce Compare May 3, 2022 09:54
kfessel
kfessel requested changes May 5, 2022
View reviewed changes
Copy link
Contributor

@kfessel kfessel left a comment •
edited
Loading

Choose a reason for hiding this comment

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

The current behavior isn't to not set the interface but it is set to 0 (as it is done for IPv4) the same is happening for non link-local addresses. (this behavior is currently untested for IPv6, but tested for IPv4 in unittests/sock_util)
With this PR non link-local addresses without interface return EINVAL (they did not before).
0 seems a sane choice to me, if there is just one interface (tell me if I am wrong).

Just to repeat the current behavior is [fe80::90a7:a6ff:fe4b:2e32] -> ep.netif=0, ep.port=0, ep.addr set value

@benpicco benpicco changed the title sys/net/sock_util: don't require interface if there is only a single one sys/net/sock_util: don't require interface for link-local addr if there is only a single one May 5, 2022
@benpicco benpicco changed the title sys/net/sock_util: don't require interface for link-local addr if there is only a single one sys/net/sock_util: don't require interface for link-local addr if there is only a single interface May 5, 2022
@benpicco
Copy link
Contributor Author

benpicco commented May 5, 2022

With this PR non link-local addresses without interface return EINVAL (they did not before).

huh, that would be a bug - this PR only applies to IPv6 link-local addresses, behavior for all other address types remains unchanged.

@kfessel
Copy link
Contributor

kfessel commented May 5, 2022

With this PR non link-local addresses without interface return EINVAL (they did not before).

huh, that would be a bug - this PR only applies to IPv6 link-local addresses, behavior for all other address types remains unchanged.

oops that wasn't the cause when i tested with unittests/sock_util (adding the test for the current untested behavior). The cause was unittests/sock_util runs without having any netif this will lead to EINVAL.

@kfessel kfessel mentioned this pull request May 5, 2022
Merged
@benpicco benpicco mentioned this pull request May 5, 2022
Merged
@benpicco
Copy link
Contributor Author

benpicco commented May 9, 2022

superseded by #18065

@benpicco benpicco closed this May 9, 2022
@benpicco benpicco deleted the sock_util-default_if branch May 9, 2022 22:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@HendrikVE HendrikVE HendrikVE requested changes

@kfessel kfessel kfessel requested changes

@miri64 miri64 Awaiting requested review from miri64 miri64 is a code owner

@haukepetersen haukepetersen Awaiting requested review from haukepetersen

@PeterKietzmann PeterKietzmann Awaiting requested review from PeterKietzmann PeterKietzmann is a code owner

@cgundogan cgundogan Awaiting requested review from cgundogan

@fjmolinas fjmolinas Awaiting requested review from fjmolinas

@fabian18 fabian18 Awaiting requested review from fabian18

@smlng smlng Awaiting requested review from smlng

@leandrolanzieri leandrolanzieri Awaiting requested review from leandrolanzieri leandrolanzieri is a code owner

@aabadie aabadie Awaiting requested review from aabadie aabadie is a code owner

@MichelRottleuthner MichelRottleuthner Awaiting requested review from MichelRottleuthner MichelRottleuthner is a code owner

Assignees
No one assigned
Labels
Area: network Area: Networking Area: sys Area: System Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@benpicco @kfessel @HendrikVE