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.

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

gnrc_netif: fix timeout in gnrc_netif_ipv6_wait_for_global_address() #21137

Conversation

benpicco
Copy link
Contributor

Contribution description

The timeout value in gnrc_netif_ipv6_wait_for_global_address() would overflow for large timeouts, leading to an immediate return.

Instead of doing arithmetic, set a proper timer - or don't if the timeout is UINT32_MAX.

Testing procedure

Issues/PRs references

@github-actions github-actions bot added Area: network Area: Networking Area: sys Area: System labels Jan 16, 2025
@benpicco benpicco force-pushed the gnrc_netif_ipv6_wait_for_global_address-fix_timeout branch from c5ccefa to a95bd50 Compare January 16, 2025 15:07
@benpicco benpicco added the Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) label Jan 16, 2025
@benpicco benpicco requested review from kfessel and maribu January 16, 2025 15:09
@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 Jan 16, 2025
@riot-ci
Copy link

riot-ci commented Jan 16, 2025

Murdock results

✔️ PASSED

a95bd50 gnrc_netif: fix timeout in gnrc_netif_ipv6_wait_for_global_address()

Success Failures Total Runtime
10270 0 10271 15m:16s

Artifacts

Copy link
Contributor

@kfessel kfessel left a comment

Choose a reason for hiding this comment

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

looks very good to me.
very good use of ztimer -- reducing the number of ztimer_now.

I assume you tested this.
In case I would trust your testing and approve.

if (ztimer_msg_receive_timeout(ZTIMER_MSEC, &m, timeout_ms) < 0) {
if (timeout_ms == UINT32_MAX) {
msg_receive(&m);
} else if (ztimer_msg_receive_timeout(ZTIMER_MSEC, &m, timeout_ms) < 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
} else if (ztimer_msg_receive_timeout(ZTIMER_MSEC, &m, timeout_ms) < 0) {
}
else if (ztimer_msg_receive_timeout(ZTIMER_MSEC, &m, timeout_ms) < 0) {

according to RIOT coding style

Copy link
Contributor

Choose a reason for hiding this comment

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

since both styles are already in this file, this can be ignored

Copy link
Contributor

@MrKevinWeiss MrKevinWeiss left a comment

Choose a reason for hiding this comment

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

Tasty bug fix before release. Some trust in @kfessel and @benpicco testing.

@MrKevinWeiss MrKevinWeiss added this pull request to the merge queue Jan 20, 2025
Merged via the queue into RIOT-OS:master with commit 8302223 Jan 20, 2025
30 checks passed
@benpicco benpicco deleted the gnrc_netif_ipv6_wait_for_global_address-fix_timeout branch January 20, 2025 13:40
@MrKevinWeiss MrKevinWeiss added this to the Release 2025.01 milestone Jan 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: network Area: Networking Area: sys Area: System 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.

4 participants