Skip to content

Add an NV_SEED test to the config adjustment script #6509

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
Merged
merged 2 commits into from
Apr 10, 2018
Merged

Add an NV_SEED test to the config adjustment script #6509

merged 2 commits into from
Apr 10, 2018

Conversation

k-stachowiak
Copy link
Contributor

@k-stachowiak k-stachowiak commented Mar 29, 2018 •
edited
Loading

Description

This PR adds a check for the NV_SEED option in the script that imports the Mbed TLS library.

Pull request type

[x] Fix
[ ] Refactor
[ ] New target
[ ] Feature
[ ] Breaking change

Patater
Patater reviewed Mar 29, 2018
View reviewed changes
features/mbedtls/importer/adjust-config.sh Outdated
@@ -66,7 +66,8 @@ add_code
"#endif\n" \
"\n" \
"#if defined(MBEDTLS_SSL_TLS_C) && !defined(MBEDTLS_TEST_NULL_ENTROPY) && \\\\\n" \
" !defined(MBEDTLS_ENTROPY_HARDWARE_ALT)\n" \
" !defined(MBEDTLS_ENTROPY_HARDWARE_ALT) && \\\\\n" \
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need so many backslashes here, do we?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without them we barely make it at the line being 79 characters wide. I will change this.

Copy link

Choose a reason for hiding this comment

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

Four \\\\s are to retain one \ used as line continuation escape character in multi line macro.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's understood, but we didn't have them before. I'm happy with the currently proposed change.

Copy link

Choose a reason for hiding this comment

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

They were introduced because we were extending the macro with another line.
Now the change is made in the same line. Hence not needed.

@Patater
Copy link
Contributor

Patater commented Apr 4, 2018

What testing has been done?

Patater
Patater approved these changes Apr 4, 2018
View reviewed changes
Copy link
Contributor

@Patater Patater left a comment

Choose a reason for hiding this comment

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

Approved, pending testing.

@mazimkhan
Copy link

Looks good to me.

@Patater
Copy link
Contributor

Patater commented Apr 4, 2018

@Alonof can you confirm any testing done on this and the results? Thanks.

@Alonof
Copy link
Contributor

Alonof commented Apr 8, 2018

Hi,
I tested locally on my K64F board, I disable the H/W TRNG and used the nv_seed
and it works,
I also printed the output of the read function to make sure that the random is working

@k-stachowiak k-stachowiak changed the title DO NOT MERGE! Add an NV_SEED test to the config adjustment script Add an NV_SEED test to the config adjustment script Apr 9, 2018
@k-stachowiak
Copy link
Contributor Author

@cmonr Could you please remove the do not merge label? The PR has been reviewed and is ready to go.

@adbridge
Copy link
Contributor

adbridge commented Apr 9, 2018

@Alonof Do you have test results that you could actually paste into this PR please ?

@adbridge
Copy link
Contributor

adbridge commented Apr 9, 2018

/morph build

@adbridge adbridge self-requested a review April 9, 2018 16:25
adbridge
adbridge approved these changes Apr 9, 2018
View reviewed changes
Copy link
Contributor

@adbridge adbridge left a comment

Choose a reason for hiding this comment

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

LGTM

@mbed-ci
Copy link

mbed-ci commented Apr 9, 2018

Build : SUCCESS

Build number : 1689
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/6509/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

@mbed-ci
Copy link

mbed-ci commented Apr 9, 2018

Exporter Build : SUCCESS

Build number : 1321
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/6509/

@mbed-ci
Copy link

mbed-ci commented Apr 10, 2018

Test : SUCCESS

Build number : 1483
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/6509/1483

@Alonof
Copy link
Contributor

Alonof commented Apr 10, 2018 •
edited
Loading

@adbridge
I don't have any specific test results I run them locally.
can you please advice on which tests to run and I shall run them( are PAL tests for TLS are OK)
also on my tests, I injected SOTP value for initial seed

@adbridge
Copy link
Contributor

@Patater @mazimkhan Are there any specific tests you think should be run against this PR ?

@mazimkhan
Copy link

@Patater @mazimkhan Are there any specific tests you think should be run against this PR ?

Its a minor script change and it has been manually tested by @Alonof. That is enough for this PR.

@0xc0170 0xc0170 merged commit 495ae06 into ARMmbed:master Apr 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Patater Patater Patater approved these changes

@mazimkhan mazimkhan mazimkhan approved these changes

@simonbutcher simonbutcher simonbutcher approved these changes

@adbridge adbridge adbridge approved these changes

Assignees
No one assigned
Labels
release-version: 5.8.3
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@k-stachowiak @Patater @mazimkhan @Alonof @adbridge @mbed-ci @simonbutcher @cmonr @0xc0170