Skip to content

random: add random_bytes() function#7390

Merged
jnohlgard merged 1 commit intoRIOT-OS:masterfrom
tobhe:random
Feb 9, 2018
Merged

random: add random_bytes() function#7390
jnohlgard merged 1 commit intoRIOT-OS:masterfrom
tobhe:random

Conversation

@tobhe
Copy link
Copy Markdown
Contributor

@tobhe tobhe commented Jul 20, 2017

Adds a random_bytes() function allowing to read a specified amount of bytes to a buffer. This might be useful, as currently it is only possible to read 32-bit integers from the random module.
Possible use cases include #7393.

@tobhe tobhe changed the title random: added random_bytes() function random: add random_bytes() function Jul 20, 2017
@jnohlgard
Copy link
Copy Markdown
Member

please provide a description of the purpose of this PR, and any other relevant information.

uint32_t last;

while(size > sizeof(uint32_t)) {
*((uint32_t*)(buf+count)) = tinymt32_generate_uint32(&_random);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This code may (/will) cause alignment errors on ARM systems, as alignof(uint8_t) <= alignof(uint32_t).

Copy link
Copy Markdown
Contributor Author

@tobhe tobhe Jul 21, 2017

Choose a reason for hiding this comment

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

Thanks for the hint. Is there any proper way to avoid this?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

One way would be to round the buffer pointer up to nearest 32 bit word and perform the loop. Finally generate two more words of random and use them to fill the leading and trailing bytes of the buffer.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I believe this should be fixed in the newest commit

@miri64 miri64 added the Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation label Sep 19, 2017
@jnohlgard
Copy link
Copy Markdown
Member

Needs rebase

@tobhe tobhe force-pushed the random branch 2 times, most recently from 4ac5e53 to bd33a21 Compare September 19, 2017 13:30
@tobhe
Copy link
Copy Markdown
Contributor Author

tobhe commented Sep 19, 2017

@gebart done

@jnohlgard jnohlgard added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Oct 23, 2017
Copy link
Copy Markdown
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.

This looks fine except for the stray extra include.

However, we are not testing this functionality in any of the existing tests, I suggest we create a follow-up PR after this PR and #7421 have been merged to update the implementation in #7421 to use random_bytes. That way, random_bytes will be tested implicitly by the unit tests in #7421.

Please remove the unused include, then squash

#define RANDOM_H

#include <inttypes.h>
#include <stdlib.h>
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is not used within this header if I am not mistaken.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

absolutely right, not sure why I put it there

@tobhe
Copy link
Copy Markdown
Contributor Author

tobhe commented Dec 8, 2017

@gebart: done, combining the test with #7421 sounds like a good idea to me.

@tobhe
Copy link
Copy Markdown
Contributor Author

tobhe commented Dec 19, 2017

turns out the include probably wasn't as useless as we thought, I added #include <stddef.h> for the size_t argument in random_bytes().

Copy link
Copy Markdown
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.

This looks fine now, ACK.
Let's implement the test in a separate PR

@@ -27,6 +27,7 @@
#define RANDOM_H

#include <inttypes.h>
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

not really this PR's fault but this should be stdint.h instead. stdint provides the uintxx_t which I assume is the reason for this header. inttypes in addition provides the PRIx32 etc printf format macros as well as some more stuff which is not used by this API header. (You don't have to address this comment in this PR)

@jnohlgard
Copy link
Copy Markdown
Member

and go

@jnohlgard jnohlgard merged commit 2cc07f7 into RIOT-OS:master Feb 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants