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

DHCP: Add an upper bound to the renew/rebind timeout in RetryConfig #835

Merged
merged 1 commit into from
Dec 23, 2023

Conversation

JarredAllen
Copy link
Contributor

Description

This lets the user set an upper bound on the duration between times smoltcp sends a renew or rebind by setting a max_renew_timeout field to DHCP RetryConfig struct, which defaults to not imposing a cap unless the user explicitly sets one.

Design questions

Use of this feature doesn't follow the official DHCP spec, but the RetryConfig struct already has a min_renew_timeout field which breaks the same spec by allowing the user to change the lower-bound imposed, so I think this is reasonable to add. Feel free to close this PR if you disagree.

I also added #[non_exhaustive] to the RetryConfig struct because this addition will necessarily be backwards incompatible and ineligible for inclusion until v0.11.0, but marking it as #[non_exhaustive] will allow anyone else in the future who wants more configuration options to add them without breaking backwards-compatibility. I can remove this attribute and let any future additions also require a major change instead.

I also didn't put any checks for the user putting max_renew_duration smaller than the existing min_renew_duration. If they do, then they'll silently get max_renew_duration as the chosen value every time. I think this is a reasonable response to an unlikely scenario, but I can do something else instead if you prefer.

Validation

The existing unit tests cover logic in the case where no upper bound is set. I added a new unit test that covers the user setting a custom upper- and lower- bound to ensure that this behavior acts correctly.

I also have a firmware setup which uses this DHCP client that I switched to using this branch and set the maximum, and observed it capping at the maximum like expected.

@codecov
Copy link

codecov bot commented Aug 23, 2023

Codecov Report

Merging #835 (3755e86) into main (01e1acb) will increase coverage by 0.07%.
Report is 3 commits behind head on main.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #835      +/-   ##
==========================================
+ Coverage   79.52%   79.59%   +0.07%     
==========================================
  Files          78       78              
  Lines       27762    27816      +54     
==========================================
+ Hits        22078    22141      +63     
+ Misses       5684     5675       -9     
Files Changed Coverage Δ
src/time.rs 86.41% <ø> (ø)
src/socket/dhcpv4.rs 86.32% <100.00%> (+0.71%) ⬆️

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Member

@Dirbaio Dirbaio left a comment

Choose a reason for hiding this comment

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

thank you!

@Dirbaio Dirbaio added this pull request to the merge queue Dec 23, 2023
Merged via the queue into smoltcp-rs:main with commit 3d3fea4 Dec 23, 2023
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants