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

Add an NV_SEED test to the config adjustment script #6509

Merged
merged 2 commits into from Apr 10, 2018

Conversation

Projects
None yet
9 participants
@k-stachowiak
Contributor

k-stachowiak commented Mar 29, 2018

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

@@ -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" \

This comment has been minimized.

@Patater

Patater Mar 29, 2018

Contributor

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

This comment has been minimized.

@k-stachowiak

k-stachowiak Mar 29, 2018

Contributor

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

This comment has been minimized.

@mazimkhan

mazimkhan Apr 4, 2018

Contributor

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

This comment has been minimized.

@Patater

Patater Apr 4, 2018

Contributor

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

This comment has been minimized.

@mazimkhan

mazimkhan Apr 4, 2018

Contributor

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

@cmonr cmonr added the do not merge label Mar 29, 2018

@Patater

This comment has been minimized.

Contributor

Patater commented Apr 4, 2018

What testing has been done?

@Patater

Patater approved these changes Apr 4, 2018

Approved, pending testing.

@mazimkhan

This comment has been minimized.

Contributor

mazimkhan commented Apr 4, 2018

Looks good to me.

@Patater

This comment has been minimized.

Contributor

Patater commented Apr 4, 2018

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

@Alonof

This comment has been minimized.

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 from DO NOT MERGE! Add an NV_SEED test to the config adjustment script to Add an NV_SEED test to the config adjustment script Apr 9, 2018

@k-stachowiak

This comment has been minimized.

Contributor

k-stachowiak commented Apr 9, 2018

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

@adbridge adbridge added needs: CI and removed do not merge labels Apr 9, 2018

@adbridge

This comment has been minimized.

Contributor

adbridge commented Apr 9, 2018

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

@adbridge

This comment has been minimized.

Contributor

adbridge commented Apr 9, 2018

/morph build

@adbridge adbridge self-requested a review Apr 9, 2018

@adbridge

LGTM

@mbed-ci

This comment has been minimized.

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

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@Alonof

This comment has been minimized.

Contributor

Alonof commented Apr 10, 2018

@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

This comment has been minimized.

Contributor

adbridge commented Apr 10, 2018

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

@mazimkhan

This comment has been minimized.

Contributor

mazimkhan commented Apr 10, 2018

@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

11 checks passed

AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-mbed2-build build completed
Details
ci-morph-test test completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 8804 cycles (-8297 cycles)
Details
travis-ci/littlefs Passed, code size is 10092B (+0.00%)
Details
travis-ci/tools Local tools testing has passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment