Skip to content

Conversation

douglasbakkum
Copy link
Member

No description provided.

src/random.c Outdated
{
uint32_t i = 0;
#ifndef TESTING
uint32_t l = 0;
Copy link

@x1ddos x1ddos Jul 26, 2018

Choose a reason for hiding this comment

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

Maybe do the opposite, #ifdef TESTING? It seem when in TESTING it's just a few lines at the bottom, while most of the func is unnecessary indented. Just saying, it would improve readability of random_bytes overall, imho. But maybe not in this change though.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll do that and agree readability will improve, but what do you mean by 'unnecessary indented'? (I don't see how the indentation changes.)

Copy link

Choose a reason for hiding this comment

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

Ah sorry, my mistake. Please, ignore the indentation part. I was thinking in a different language. :)

// Add entropy from ataes RNG
while (len > l) {
if (update_seed) {
ret = ataes_process(ataes_cmd_up, sizeof(ataes_cmd_up), ataes_ret, sizeof(ataes_ret));
Copy link

Choose a reason for hiding this comment

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

Maybe also move ataes_cmd and other vars closer to this loop? Otherwise, one needs to scroll up and down a couple times now to see where those are declared and what the values are.

Copy link
Member Author

Choose a reason for hiding this comment

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

ack

src/random.c Outdated
ret = ataes_process(ataes_cmd, sizeof(ataes_cmd), ataes_ret, sizeof(ataes_ret));
}
if (ret == DBB_OK && ataes_ret[0] && !ataes_ret[1]) {
for (i = 0; i < MIN(len - l, 16); i++) {
Copy link

Choose a reason for hiding this comment

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

The number 16 is important enough that it deserves a #define imho.

Copy link
Member Author

Choose a reason for hiding this comment

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

ACK.
There is a future commit that adds some ATAES-specific defines. I'll include this in that PR.

src/random.c Outdated
}
#endif
// Add entropy from ataes RNG
while (len > l) {
Copy link

Choose a reason for hiding this comment

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

For the first few minutes I thought it was len > 1, i.e. len is greater than one, not the var l. Change the name to something like n to avoid ambiguity? Maybe it's just me.

Copy link
Member Author

Choose a reason for hiding this comment

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

ACK

@x1ddos
Copy link

x1ddos commented Jul 27, 2018

LGTM

@douglasbakkum douglasbakkum merged commit 0672207 into BitBoxSwiss:master Jul 28, 2018
douglasbakkum pushed a commit that referenced this pull request Jul 28, 2018
0672207 refactor random.c; include extra source of entropy from usersig (djb)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants