Skip to content

Add platform setup and teardown calls to mbedtls tests #6749

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

Merged
merged 2 commits into from
May 17, 2018
Merged

Add platform setup and teardown calls to mbedtls tests #6749

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1938faa
Add platform setup and teardown calls to mbedtls tests
Apr 26, 2018
29515c5
Update logs and remove unnecessary defines for Mbed TLS tests
May 8, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 20 additions & 1 deletion TESTS/mbedtls/multi/main.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,12 @@

#include "mbedtls/sha256.h"

#if defined(MBEDTLS_PLATFORM_C)
#include "mbedtls/platform.h"
#else
#include <stdio.h>
Copy link

Choose a reason for hiding this comment

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

Is there a reason to include this stuff in the #else block.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be consistent with other examples, and to allow compilation on platforms without PLATFORM_C define.

Copy link

Choose a reason for hiding this comment

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

@hanno-arm made a more specific comment #6749 (review)

#define mbedtls_printf printf
#endif

using namespace utest::v1;

Expand Down Expand Up @@ -163,5 +169,18 @@ utest::v1::status_t greentea_test_setup(const size_t number_of_cases) {
Specification specification(greentea_test_setup, cases, greentea_test_teardown_handler);

int main() {
Harness::run(specification);
int ret = 0;
#if defined(MBEDTLS_PLATFORM_C)
mbedtls_platform_context platform_ctx;
Copy link

Choose a reason for hiding this comment

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

mbedtls_platform_context can be defined platform specific. We don't know if it will fit stack of an embedded app. It may be more robust to declare it static.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This declaration is consistent with other examples, and in my opinion should be up to the implementer to change, if the stack is small. I would say that this is a standard place to have the context.

Copy link

@mazimkhan mazimkhan May 8, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Are you suggesting that if an implementation introduces mbedtls_platform_context that is inappropriate for the stack then they should change all tests. If we anticipate that, we can avoid it by creating it static or on heap.

Copy link

Choose a reason for hiding this comment

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

@AndrzejKurek Do you mean Mbed TLS internal example programs or other test programs in the Mbed OS repository?

Copy link
Contributor Author

@AndrzejKurek AndrzejKurek May 8, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@hanno-arm I mean our examples. Mbed OS repository examples present mixed approach to how and where are dependencies declared.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mazimkhan How do we know if someone doesn't prefer to have it on the stack instead? I can introduce your change, but I feel like it's a matter of preferences.

Copy link

Choose a reason for hiding this comment

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

It is about robustness and life time. The life time of platform context is process. It will live as a singleton instance throughout the process. Hence no harm in having it ``static``` or global.

Secondly, it will be robust to have it statically allocated since memory requirement will be checked at compile time. With current implementation it may fail at run time and will require debugging and code change to adjust for the application.

Copy link

Choose a reason for hiding this comment

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

I don't see the reason to have a platform structure in the first place, as opposed to having parameterless functions mbedtls_platform_setup() and mbedtls_platform_teardown() and leaving it to the implementation to allocate and maintain platform specific globals. Also, how would the platform context be actually used in the way it currently works? E.g., how would a custom printf implementation have access to it?

This is an independent design discussion, though, that we should probably have outside of this PR.

if((ret = mbedtls_platform_setup(&platform_ctx))!= 0)
{
mbedtls_printf("Mbed TLS multitest failed! mbedtls_platform_setup returned %d\n", ret);
return 1;
}
#endif
ret = (Harness::run(specification) ? 0 : 1);
#if defined(MBEDTLS_PLATFORM_C)
mbedtls_platform_teardown(&platform_ctx);
#endif
return ret;
}
20 changes: 14 additions & 6 deletions TESTS/mbedtls/selftest/main.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,12 +39,7 @@ using namespace utest::v1;
#include "mbedtls/platform.h"
#else
#include <stdio.h>
#include <stdlib.h>
#define mbedtls_printf printf
#define mbedtls_snprintf snprintf
#define mbedtls_exit exit
#define MBEDTLS_EXIT_SUCCESS EXIT_SUCCESS
#define MBEDTLS_EXIT_FAILURE EXIT_FAILURE
#endif

#define MBEDTLS_SELF_TEST_TEST_CASE(self_test_function) \
Expand Down Expand Up @@ -97,6 +92,19 @@ utest::v1::status_t test_setup(const size_t num_cases) {
Specification specification(test_setup, cases);

int main() {
return !Harness::run(specification);
int ret = 0;
#if defined(MBEDTLS_PLATFORM_C)
mbedtls_platform_context platform_ctx;
if((ret = mbedtls_platform_setup(&platform_ctx))!= 0)
{
mbedtls_printf("Mbed TLS selftest failed! mbedtls_platform_setup returned %d\n", ret);
return 1;
}
#endif
ret = (Harness::run(specification) ? 0 : 1);
#if defined(MBEDTLS_PLATFORM_C)
mbedtls_platform_teardown(&platform_ctx);
#endif
return ret;
}