Skip to content

shell/gnrc_icmpv6_echo: fix incorrect use of gnrc_netif_highlander() #13737

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
Merged
merged 1 commit into from
Mar 27, 2020
Merged

shell/gnrc_icmpv6_echo: fix incorrect use of gnrc_netif_highlander() #13737

merged 1 commit into from
Mar 27, 2020

Conversation

benpicco
Copy link
Contributor

Contribution description

There are instances where gnrc_netif_highlander() is used to check if there is only one interface.
The gnrc_netif_highlander() however only returns true if there can only be one interface.
It will return false if there is only one interface, but more could be configured.

Testing procedure

see #12994 (comment)

Issues/PRs references

alternative to #13736

@benpicco benpicco added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Area: network Area: Networking labels Mar 27, 2020
@benpicco benpicco requested a review from jia200x March 27, 2020 12:50
@benpicco
Copy link
Contributor Author

Following #13736 (comment) I dropped all but the shell/gnrc_icmpv6_echo fix.

@benpicco benpicco changed the title fix incorrect use of gnrc_netif_highlander() shell/gnrc_icmpv6_echo: fix incorrect use of gnrc_netif_highlander() Mar 27, 2020
miri64
miri64 reviewed Mar 27, 2020
View reviewed changes
miri64
miri64 requested changes Mar 27, 2020
View reviewed changes
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.

See discussion above

miri64
miri64 approved these changes Mar 27, 2020
View reviewed changes
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.

With my suggested fix it works as expected. Please address and squash so we can get this merged ASAP.

@miri64
Copy link
Member

miri64 commented Mar 27, 2020

Please also fix commit message and PR title for something more in line with the current change.

@benpicco benpicco force-pushed the highlander-fix branch 2 times, most recently from ed2f370 to 158ca59 Compare March 27, 2020 13:36
@benpicco
Copy link
Contributor Author

Please also fix commit message and PR title for something more in line with the current change.

shell/gnrc_icmpv6_echo: fix replace incorrect use of gnrc_netif_highlander()?

@benpicco
shell/gnrc_icmpv6_echo: replace incorrect use of gnrc_netif_highlander()
e1ce24a 
We want to check if there *is* only one interface, not if there *can*
be no more than one interface here.
miri64
miri64 approved these changes Mar 27, 2020
View reviewed changes
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.

Re-ACK. Tested the current state with and without GNRC_NETIF_SINGLE defined. Please squash.

@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 Mar 27, 2020
@miri64 miri64 merged commit 155dd30 into RIOT-OS:master Mar 27, 2020
@benpicco benpicco deleted the highlander-fix branch March 27, 2020 14:54
@miri64 miri64 mentioned this pull request Mar 27, 2020
Closed
@leandrolanzieri leandrolanzieri added the Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer label Apr 3, 2020
@leandrolanzieri leandrolanzieri added this to the Release 2020.04 milestone Apr 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@miri64 miri64 miri64 approved these changes

@cgundogan cgundogan Awaiting requested review from cgundogan

@haukepetersen haukepetersen Awaiting requested review from haukepetersen

@PeterKietzmann PeterKietzmann Awaiting requested review from PeterKietzmann

@jia200x jia200x Awaiting requested review from jia200x

Assignees
No one assigned
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 Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Milestone
Release 2020.04
Development

Successfully merging this pull request may close these issues.
3 participants
@benpicco @miri64 @leandrolanzieri