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.

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

Move part of timing module out of the library #4640

Merged
merged 7 commits into from
Jun 18, 2021
Merged

Move part of timing module out of the library #4640

merged 7 commits into from
Jun 18, 2021

Conversation

TRodziewicz
Copy link
Contributor

@TRodziewicz TRodziewicz commented Jun 10, 2021 •
edited
Loading

Signed-off-by: TRodziewicz tomasz.rodziewicz@mobica.com

Description

  • mbedtls_timing_hardclock(): move to benchmark.c.
  • mbedtls_hardclock_poll(): remove.
  • mbedtls_timing_get_timer(): add warning “may change without notice”
  • mbedtls_set_alarm(): move to benchmark.c.
  • mbedtls_timing_self_test(): remove.

Fixes #4083

Status

IN DEVELOPMENT

Requires Backporting

NO

Migrations

NO

Todos

  • Tests
  • Documentation
  • Changelog updated

@TRodziewicz TRodziewicz added enhancement needs-ci Needs to pass CI tests mbedtls-3 size-s Estimated task size: small (~2d) labels Jun 10, 2021
@TRodziewicz TRodziewicz self-assigned this Jun 10, 2021
@TRodziewicz TRodziewicz force-pushed the move_part_of_timing_module_out_of_the_library_and_to_test branch from f6a917f to d2591c9 Compare June 11, 2021 13:51
@TRodziewicz TRodziewicz added needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review and removed needs-ci Needs to pass CI tests labels Jun 11, 2021
@gilles-peskine-arm gilles-peskine-arm self-requested a review June 14, 2021 18:20
@TRodziewicz TRodziewicz added needs-work and removed needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review labels Jun 15, 2021
@tomasz-rodziewicz-mobica
Move part of timing module out of the library
d854083 
Signed-off-by: TRodziewicz <tomasz.rodziewicz@mobica.com>
@tomasz-rodziewicz-mobica
Fixing test_clang_opt (asm not defined)
90f304f 
Signed-off-by: TRodziewicz <tomasz.rodziewicz@mobica.com>
@tomasz-rodziewicz-mobica
Make static all the functions moved to bechmark.c
9a9609e 
Signed-off-by: TRodziewicz <tomasz.rodziewicz@mobica.com>
@tomasz-rodziewicz-mobica
Remove the mbedtls_printf and FAIL macros
5bbbb5e 
Signed-off-by: TRodziewicz <tomasz.rodziewicz@mobica.com>
@tomasz-rodziewicz-mobica
Addition of the migration guide and change log files
9c90226 
Signed-off-by: TRodziewicz <tomasz.rodziewicz@mobica.com>
@TRodziewicz TRodziewicz force-pushed the move_part_of_timing_module_out_of_the_library_and_to_test branch from d2591c9 to 9c90226 Compare June 15, 2021 13:49
@TRodziewicz TRodziewicz added needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review and removed needs-work labels Jun 15, 2021
gilles-peskine-arm
gilles-peskine-arm requested changes Jun 15, 2021
View reviewed changes
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm left a comment

Choose a reason for hiding this comment

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

Looks good to me apart from some documentation issues.

ChangeLog.d/issue4083.txt Outdated Show resolved Hide resolved
ChangeLog.d/issue4083.txt Outdated Show resolved Hide resolved
docs/3.0-migration-guide.d/move_part_of_timing_module_out_of_the_library.md Outdated Show resolved Hide resolved
docs/3.0-migration-guide.d/move_part_of_timing_module_out_of_the_library.md Outdated Show resolved Hide resolved
include/mbedtls/config.h Outdated Show resolved Hide resolved
include/mbedtls/timing.h Outdated Show resolved Hide resolved
@tomasz-rodziewicz-mobica
Documentation rewording
15a7b73 
Signed-off-by: TRodziewicz <tomasz.rodziewicz@mobica.com>
@daverodgman daverodgman self-requested a review June 17, 2021 14:23
daverodgman
daverodgman previously approved these changes Jun 17, 2021
View reviewed changes
Copy link
Contributor

@daverodgman daverodgman left a comment

Choose a reason for hiding this comment

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

LGTM, optional very minor comment

library/timing.c Outdated
#else /* _WIN32 && !EFIX64 && !EFI32 */

/**
Copy link
Contributor

Choose a reason for hiding this comment

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

Possibly nicer to move the comment block and the first line of the function (i.e. the line "unsigned long mbedtls_timing_get_timer...") up outside of the #if defined ... blocks, so that it is clearly describing both implementations rather than just the non-WIndows case. This is very minor though.

@ronald-cron-arm ronald-cron-arm added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review labels Jun 17, 2021
@mpg
Copy link
Contributor

mpg commented Jun 18, 2021

Out of curiosity I was having a look at what remains in timing.h, and noticed it still declares extern volatile int mbedtls_timing_alarmed; which I think is unnecessary now that mbedtls_set_alarm() was removed. Can you remove that declaration too? Thanks!

@mpg mpg added needs-work and removed approved Design and code approved - may be waiting for CI or backports labels Jun 18, 2021
@tomasz-rodziewicz-mobica
Removing global variable and moving variant function comment block
963bb81 
Signed-off-by: TRodziewicz <tomasz.rodziewicz@mobica.com>
@gilles-peskine-arm gilles-peskine-arm added needs-review Every commit must be reviewed by at least two team members, and removed needs-work labels Jun 18, 2021
@TRodziewicz TRodziewicz requested a review from daverodgman June 18, 2021 12:58
@daverodgman daverodgman added needs-ci Needs to pass CI tests and removed needs-review Every commit must be reviewed by at least two team members, needs-ci Needs to pass CI tests labels Jun 18, 2021
@daverodgman daverodgman merged commit 8c8166a into Mbed-TLS:development Jun 18, 2021
@bruvzg bruvzg mentioned this pull request Dec 21, 2022
Merged
@AndrzejKurek AndrzejKurek mentioned this pull request Jan 16, 2023
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gilles-peskine-arm gilles-peskine-arm gilles-peskine-arm approved these changes

@daverodgman daverodgman daverodgman approved these changes

Assignees

@TRodziewicz TRodziewicz

Labels
enhancement size-s Estimated task size: small (~2d)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Move part of timing module out of the library and to test
6 participants
@TRodziewicz @mpg @daverodgman @gilles-peskine-arm @ronald-cron-arm @tomasz-rodziewicz-mobica