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

Basic rand infrastructure. #547

Merged
merged 2 commits into from
Oct 14, 2021
Merged

Basic rand infrastructure. #547

merged 2 commits into from
Oct 14, 2021

Conversation

Dirbaio
Copy link
Member

@Dirbaio Dirbaio commented Oct 11, 2021

See previous discussion. Opening a separate PR so it can be discussed separately.

  • Add smoltcp::rand.
    • On std targets, OsRng is used by default.
    • The user can supply a custom impl by enabling the rand-custom-impl Cargo feature and using the rand_custom_impl!() macro.
    • Specifying a custom impl is mandatory when std is not enabled.
  • Make TCP initial sequence numbers actually random.

@Dirbaio Dirbaio mentioned this pull request Oct 11, 2021
Merged
11 tasks
src/rand.rs Outdated Show resolved Hide resolved
On `std` targets, `OsRng` is used by default. The user can supply a custom impl
by enabling the `rand-custom-impl` Cargo feature and using the `rand_custom_impl!()` macro.
Specifying a custom impl is mandatory when `std` is not enabled.
Copy link
Contributor

@ryan-summers ryan-summers left a comment

Choose a reason for hiding this comment

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

One comment, otherwise looks good to me :) Nice approach to avoid templatizing everything over some impl RandCore

Ok(())
}

fn random_seq_no() -> TcpSeqNumber {
#[cfg(test)]
return TcpSeqNumber(10000);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the intent to not implement a "bad" rand generator in test so that a user doesn't accidentally enable it for their app? It seems like we should be able to use the OS's rand for test purposes?

Copy link
Member Author

Choose a reason for hiding this comment

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

Many tests rely on the starting sequence number to be 10000. Changing them to make them work with a random starting seq number is annoying, and wouldn't really add much value.

One alternative I tried is making the actual RNG a mock when building in test mode, so the tests can "magically influence" what numbers come out of the RNG. The problem is tests would have to be aware of how and when does TCP use the RNG, which is kind of an implementation detail (ie when does it request bytes, how many bytes does it request, are they little/big endian to form the u32 sequence number...?)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I see the limitation now. It just feels like addressing it at the tcp.rs layer may be introducing tech-debt for the future, but it looks extremely lightweight, so I'm not worried. Thanks for the explanation!

@Dirbaio
Copy link
Member Author

Dirbaio commented Oct 14, 2021

bors r+

@bors
Copy link
Contributor

bors bot commented Oct 14, 2021

Build succeeded:

@bors bors bot merged commit 9e2937e into master Oct 14, 2021
@bors bors bot deleted the rand branch October 14, 2021 11:58
@Dirbaio Dirbaio mentioned this pull request Nov 25, 2021
bors bot added a commit that referenced this pull request Dec 4, 2021
573: Simpler rand r=Dirbaio a=Dirbaio

Requires #571 

Revisits #547, because it turns out the `rand_custom_impl!` macro stuff was less nice than I thought. It's now a simple PRNG owned by Interface. The builder allows setting the random seed.

Co-authored-by: Dario Nieuwenhuis <[email protected]>
Co-authored-by: Thibaut Vandervelden <[email protected]>
Co-authored-by: bors[bot] <26634292+bors[bot]@users.noreply.github.com>
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