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.

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

PSA return status coverage script #117

Merged

Conversation

gilles-peskine-arm
Copy link
Collaborator

Add infrastructure to run unit tests and collect the return values for every PSA API function that returns psa_status_t.

./tests/scripts/psa_collect_statuses.py >statuses.txt

Status: runs ok but not very well documented and the build script is not very robust.

@gilles-peskine-arm gilles-peskine-arm added enhancement New feature or request needs: review The pull request is ready for review. This generally means that it has no known issues. needs: design review api-spec Issue or PR about the PSA specifications labels May 20, 2019
@gilles-peskine-arm gilles-peskine-arm force-pushed the psa_error_code_coverage branch from fc7e674 to 63a82cd Compare May 20, 2019 17:56
@gilles-peskine-arm gilles-peskine-arm deleted the psa_error_code_coverage branch July 18, 2019 11:49
@gilles-peskine-arm gilles-peskine-arm force-pushed the psa_error_code_coverage branch from 5e19ec0 to 569874e Compare July 18, 2019 11:56
@Patater
Copy link
Contributor

Patater commented Sep 4, 2019

This may conflict with #213

I like the idea of checking that return codes are documented. I think the API is stable enough now that having that capability will be a help and not a hindrance.

Is it possible to continue work on this?

@gilles-peskine-arm
Copy link
Collaborator Author

I'll rebase this on top of #213 if we decide to adopt it.

Are you ok with the current design? It was good enough for my goal at the time, which was to compare reachable errors with documented errors and is still relevant. But it isn't very well integrated into Mbed Crypto. The script I have for the other side, to collect documented errors, works from the PSA spec (private link: https://github.com/ARMmbed/psa-crypto/pull/227).

@Patater
Copy link
Contributor

Patater commented Sep 4, 2019

Are you ok with the current design?

It's a bit funky in that its a mix of shell in a Makefile and some Python, but it's beneficial as is. We can refactor into something better as needed.

@gilles-peskine-arm gilles-peskine-arm changed the base branch from psa-api-1.0-beta to development September 6, 2019 17:06
Add infrastructure to run unit tests and collect the return values for
every PSA API function that returns psa_status_t.

    ./tests/scripts/psa_collect_statuses.py >statuses.txt
@gilles-peskine-arm
Copy link
Collaborator Author

I rebased the changes on top of current development. I added some documentation, but I did not change the design. I think this is good enough to include, and we'll see later whether to improve the design or to leave it as a minor hack depending on how useful we think this is.

Since it needs a slightly different build, even if that's only for the
tests, make it its own component.
@gilles-peskine-arm gilles-peskine-arm removed api-spec Issue or PR about the PSA specifications needs: design review labels Sep 6, 2019
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.

LGTM

component_test_psa_collect_statuses () {
msg "build+test: psa_collect_statuses" # ~30s
scripts/config.pl full
scripts/config.pl unset MBEDTLS_MEMORY_BUFFER_ALLOC_C # slow and irrelevant
Copy link
Contributor

Choose a reason for hiding this comment

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

We may not need to add unset MBEDTLS_MEMORY_BUFFER_ALLOC_C if we sideport the removal of MBEDTLS_MEMORY_BUFFER_ALLOC_C from full config. Necessary for now.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We definitely should sideport that change. I'd expect it to come around the next time we sync from mbedtls.

Copy link
Contributor

@k-stachowiak k-stachowiak left a comment

Choose a reason for hiding this comment

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

I agree it's a bit "funky", but all the tools make sense on their own, which makes them fine by me. I made a comment regarding the build logs. If the internal build fails on a remote machine, it may be difficult to learn the failure reason. I had such a failure locally, and had to manually call make test to see, what was wrong. Otherwise, LGTM.

tests/scripts/psa_collect_statuses.py Show resolved Hide resolved
@Patater Patater removed the needs: review The pull request is ready for review. This generally means that it has no known issues. label Sep 10, 2019
@Patater Patater merged commit 4badc92 into ARMmbed:development Sep 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants