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

devfs: add /dev/urandom and /dev/hwrng #7421

Merged
merged 2 commits into from
Jun 26, 2019
Merged

devfs: add /dev/urandom and /dev/hwrng #7421

merged 2 commits into from
Jun 26, 2019

Conversation

vincent-d
Copy link
Member

This PR adds a /dev/urandom file to the devfs registry when periph_hwrng is available.

@vincent-d vincent-d requested a review from jnohlgard July 27, 2017 10:04
jnohlgard
jnohlgard requested changes Jul 27, 2017
View reviewed changes
Copy link
Member

@jnohlgard jnohlgard left a comment

Choose a reason for hiding this comment

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

See my comments inline

drivers/include/periph/hwrng.h Outdated Show resolved Hide resolved
sys/auto_init/auto_init.c Outdated Show resolved Hide resolved
sys/fs/devfs/auto_init_devfs.c Show resolved Hide resolved
@vincent-d vincent-d changed the title devfs: add /dev/urandom with hwrng devfs: add /dev/urandom and /dev/hwrng Jul 27, 2017
@vincent-d
Copy link
Member Author

  • removed hwrng_init() from auto_init
  • called hwrng_init() when opening /dev/hwrng
  • added /dev/urandom with random module

@vincent-d vincent-d added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Aug 29, 2017
@vincent-d vincent-d added this to the Release 2017.10 milestone Aug 29, 2017
@vincent-d vincent-d mentioned this pull request Nov 9, 2017
Merged
@jnohlgard
Copy link
Member

Sorry for the very long delay in reviewing this PR.
I updated the preprocessor guards with the move from FEATURES_PERIPH_xxx to MODULE_PERIPH_xxx. This PR needs a rebase, but otherwise looks fine. Tested on Mulle with both urandom and hwrng by adding FEATURES_REQUIRED += periph_hwrng and USEMODULE += random to examples/default/Makefile.
I don't know if this code is being built by any of the CI tests though, we should either add a new test in tests or add this to one of the existing tests or examples.

Feel free to squash

@vincent-d
Copy link
Member Author

squashed, and added a test in devfs unittests. I don't have hardware with me right now, so I didn't run the /dev/hwrng unittest.

jnohlgard
jnohlgard requested changes Dec 8, 2017
View reviewed changes
Copy link
Member

@jnohlgard jnohlgard left a comment

Choose a reason for hiding this comment

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

I don't think we can do any tests on the actual bytes that are produced by the read of the /dev/urandom, /dev/hwrng file, but only test to see that the return value from read is correct and that the bytes they are at least not all unchanged. We should not assume they are all changed though.

@jnohlgard
Copy link
Member

The unit tests completed successfully on Mulle, though the check needs to be changed to avoid false negatives depending on the hwrng data

@jnohlgard
Copy link
Member

This is based on a quite old base commit, could you rebase please while fixing the unit test?

@tobhe tobhe mentioned this pull request Dec 8, 2017
Merged
@vincent-d
Copy link
Member Author

Fixed unittest (amended directly) and rebased. Tested on our hardware (stm32).

@vincent-d
Copy link
Member Author

Fixed auto init call from unittests

@vincent-d vincent-d added the Area: fs Area: File systems label Feb 20, 2018
@kaspar030 kaspar030 removed this from the Release 2018.04 milestone Apr 16, 2018
jnohlgard
jnohlgard previously requested changes May 21, 2018
View reviewed changes
Copy link
Member

@jnohlgard jnohlgard left a comment

Choose a reason for hiding this comment

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

Looks good, what do you think about using a pseudomodule for the devfs nodes?

tests/unittests/tests-devfs/Makefile.include Outdated Show resolved Hide resolved
sys/fs/devfs/auto_init_devfs.c Outdated Show resolved Hide resolved
sys/fs/devfs/random-vfs.c Outdated Show resolved Hide resolved
miri64
miri64 reviewed Dec 18, 2018
View reviewed changes
tests/unittests/tests-devfs/tests-devfs.c
@@ -141,11 +143,39 @@ static void test_devfs_mount_open(void)
TEST_ASSERT_EQUAL_INT(0, res);
}

static void test_devfs_urandom(void)
Copy link
Member

Choose a reason for hiding this comment

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

Rather unrelated, but this whole test suite should go into its own application.

@miri64
Copy link
Member

miri64 commented Dec 18, 2018

Needs rebase. @gebart @vincent-d are there any plans to continue this?

@smlng smlng removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jan 10, 2019
@smlng
Copy link
Member

smlng commented Jan 10, 2019

needs rebase

@smlng
Copy link
Member

smlng commented May 29, 2019

@vincent-d still active and care to rebase this one? Just tested after rebasing, works -> would merge.
Otherwise I can take this one over - or we close this one?!

@vincent-d
Copy link
Member Author

@smlng active I am, though not much on RIOT recently ;)

If it's simply a rebase, I can do. If it needs more changes, I'm afraid won't have time.

@vincent-d
Copy link
Member Author

rebased

@smlng
Copy link
Member

smlng commented May 29, 2019

Please squash

@vincent-d
Copy link
Member Author

squashed

@smlng smlng added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines labels May 29, 2019
smlng
smlng approved these changes May 29, 2019
View reviewed changes
Copy link
Member

@smlng smlng left a comment

Choose a reason for hiding this comment

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

LGTM ACK

@smlng
Copy link
Member

smlng commented Jun 3, 2019

@gebart not sure if you are available, but if so: please recheck and ACK or dismiss you review. I'll leave this open for a few days but would then dismiss your review and merge this.

@smlng smlng dismissed jnohlgard’s stale review June 26, 2019 19:33

no response, but comments are addressed

@smlng smlng merged commit 4f5ce88 into RIOT-OS:master Jun 26, 2019
@smlng
Copy link
Member

smlng commented Jun 26, 2019

@leandrolanzieri a pointer, as you were interested and said @jcarrano might find this useful, too.

@cladmi cladmi mentioned this pull request Jul 15, 2019
Closed
77 tasks
@cladmi
Copy link
Contributor

cladmi commented Jul 15, 2019

This requires having periph_hwrng for now running unittests. Found during 2019.07-RC1 testing.

@smlng
Copy link
Member

smlng commented Jul 17, 2019

@cladmi: you refer to the fact that now fewer boards can run the unit tests, bc some do not have the required feature, right? That's kind of the problem with the monolithic unittest binary (IMHO)?!

@cladmi
Copy link
Contributor

cladmi commented Jul 17, 2019

The release specs task 01-ci Task #4 - Unittests on iotlab-m3 now fails as the unittests cannot run on iotlab-m3 anymore, one of our reference arm platforms.
The tests-core are in there.

And yes it is a problem with being monolithic. I will do a PR to split this test out for the release.

@cladmi cladmi mentioned this pull request Jul 17, 2019
Merged
@cladmi
Copy link
Contributor

cladmi commented Jul 17, 2019

#11855

@cladmi cladmi mentioned this pull request Jul 17, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@miri64 miri64 miri64 left review comments

@smlng smlng smlng approved these changes

@jnohlgard jnohlgard jnohlgard left review comments

Assignees

@jnohlgard jnohlgard

@kaspar030 kaspar030

Labels
Area: fs Area: File systems CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@vincent-d @jnohlgard @miri64 @smlng @cladmi @kaspar030