Add support for the Feature unsupported error #211
Conversation
|
Both the preceding PRs have been merged |
Add support for `MBEDTLS_ERR_PLATFORM_FEATURE_UNSUPPORTED`. If the underlying platform does not support the current feature, skip the test, instead of failing.
RonEld
force-pushed
the
unsupported_feature_error_support
branch
from
6f99326 to
55c4a6d
Compare
|
I have rebased to resolve conflicts |
There was a problem hiding this comment.
I found few code style issues, most of which I consider negligible, except for an unaligned line and redundant spaces in the middle of one of the lines.
Otherwise the PR looks good (note that I do not know, in which functions the "feature anavailable" error is acceptable, and in which hot; therefore I did not evaluate this aspect).
Additionally, i built and ran all the examples with IAR on K64F. Unfortunately, this device doesn't trigger the new behavior, but I can confirm, that the changes don't interfere with the regular execution in my environment.
benchmark/main.cpp
Outdated
| { \ | ||
| mbedtls_printf("Feature unsupported\n"); \ | ||
| break; \ | ||
| } else if (ret != 0) { \ |
There was a problem hiding this comment.
There are unnecessary spaces in this line.
benchmark/main.cpp
Outdated
| @@ -184,7 +184,11 @@ do { \ | |||
| t.stop(); \ | |||
| ms = t.read_ms(); \ | |||
| \ | |||
| if (ret != 0) { \ | |||
| if (ret == MBEDTLS_ERR_PLATFORM_FEATURE_UNSUPPORTED)\ | |||
| { \ | |||
There was a problem hiding this comment.
Code style: this brace should probably be in line with the condition below.
There was a problem hiding this comment.
I'm sorry, but I don't understand. What brace? What condition?
There was a problem hiding this comment.
In Mbed OS the braces should be on the same lines as the if condition (https://os.mbed.com/docs/mbed-os/v5.7/reference/guidelines.html#style). So it should be:
if (condition) {
rather than:
if (condition)
{
My mind may have been clouded when I wrote the comment, because I agree it isn't clear; I apologize for that. Most probably I wanted to write "... on the same line as the condition above".
benchmark/main.cpp
Outdated
| ret = mbedtls_snprintf(title, sizeof(title), "ECDSA-%s", | ||
| curve_info->name); | ||
| curve_info->name); |
There was a problem hiding this comment.
It seems that this line is not indented correctly.
benchmark/main.cpp
Outdated
| "mbedtls_snprintf(): %d\n", ret); | ||
| goto exit; | ||
| } | ||
|
|
There was a problem hiding this comment.
This empty line is redundant.
| @@ -1246,15 +1353,36 @@ MBED_NOINLINE static int benchmark_ecdh_curve22519() | |||
| mbedtls_ecdh_init(&ecdh); | |||
| mbedtls_mpi_init(&z); | |||
|
|
|||
| ret = mbedtls_snprintf(title, sizeof(title), "ECDHE-Curve25519"); | |||
There was a problem hiding this comment.
This line is misaligned.
Fix alignments and remove extra lines and spaces.
|
@k-stachowiak Thank you for the review! I have addressed your comments. As for your note:
The underlying platform can return this error on any function that it implements. For uniformity, I added a check for this error for all crypto errors, even if they don't have an alternative implementation for the time being. |
|
@RonEld Thanks for the changes that you have provided. I'm sorry I wasn't clear in my previous review; I am afraid that some more style changes should be made in this PR. What I mean is having the opening and closing braces on the same lines as the statements. To avoid confusion I linked the Mbed OS coding style paragraph in the review comment. |
Fix braces style to be like in mbed OS, K & R style.
|
@k-stachowiak Thank you for refering me to hte mbed OS coding style. |
There was a problem hiding this comment.
@RonEld Thanks for making the changes! The code looks good now.
Add support for
MBEDTLS_ERR_PLATFORM_FEATURE_UNSUPPORTED.If the underlying platform does not support the current feature,
skip the test, instead of failing.
Dependent on ARMmbed/mbed-os#8643 and Mbed-TLS/mbedtls#2054 to be merged