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

[development] Remove DHM module #9972

Open
wants to merge 11 commits into
base: development
Choose a base branch
from

Conversation

valeriosetti
Copy link
Contributor

@valeriosetti valeriosetti commented Feb 12, 2025

Description

Resolves #9956

This PR depends on:

Once that will be merged, then the merge sequence for this PR will be:

  1. [tf-psa-crypto] Remove DHM module TF-PSA-Crypto#175
  2. this one

PR checklist

  • changelog TODO
  • development PR not required because: it's this one
  • TF-PSA-Crypto PR provided [tf-psa-crypto] Remove DHM module TF-PSA-Crypto#175
  • framework PR not needed
  • 3.6 PR not required
  • 2.28 PR not required
  • tests provided: we're removing support in this PR, so we're mostly removing tests here instead of adding them.

These sample programs depend on MBEDTLS_DHM_C which is being removed, so
they should be as well.

Signed-off-by: Valerio Setti <[email protected]>
MBEDTLS_DHM_C is being removed so all its occurencies should be removed
as well.

Signed-off-by: Valerio Setti <[email protected]>
The file is being removed together with the removal of MBEDTLS_DHM_C.

Signed-off-by: Valerio Setti <[email protected]>
The file was cancelled from the tf-psa-crypto repo following the removal
of MBEDTLS_DHM_C.

Signed-off-by: Valerio Setti <[email protected]>
Signed-off-by: Valerio Setti <[email protected]>
@valeriosetti valeriosetti self-assigned this Feb 12, 2025
@valeriosetti valeriosetti added needs-review Every commit must be reviewed by at least two team members, needs-ci Needs to pass CI tests needs-reviewer This PR needs someone to pick it up for review size-s Estimated task size: small (~2d) priority-high High priority - will be reviewed soon labels Feb 12, 2025
@valeriosetti
Copy link
Contributor Author

I checked ABI-API failures and they look reasonable to me for this kind of PR.

Harry-Ramsey
Harry-Ramsey previously approved these changes Feb 13, 2025
@valeriosetti valeriosetti removed the needs-ci Needs to pass CI tests label Feb 14, 2025
Signed-off-by: Valerio Setti <[email protected]>
@@ -42,7 +42,6 @@ int main(void)
#include "mbedtls/hmac_drbg.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

We have been through some difficulties to move new files from Mbed TLS to TF-PSA-Crypto (see Mbed-TLS/TF-PSA-Crypto#164). It would be all together easier to move benchmark.c to TF-PSA-Crypto before we start modifying it. We have not changed it since the commit that was used for the split. The identifier of this commit is 8ef9323. Thus in TF-PSA-Crypto, we just have to do "git checkout 8ef9323 -- programs/test/benchmark.c" to get into TF-PSA-Crypto benchmark.c in its current version and with all of its git history. And we have an issue for the move of benchmark.c to TF-PSA-Crypto: #9971. Thus could you address #9971 as a prerequisite to the removal of the DHM module?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thus in TF-PSA-Crypto, we just have to do "git checkout 8ef9323 -- programs/test/benchmark.c" to get into TF-PSA-Crypto benchmark.c in its current version and with all of its git history.

I just tried the command: it creates the file as expected, but git log just shows the last commit (the one that I would do when adding this file), not the entire history. Am I missing something?

Copy link
Contributor

Choose a reason for hiding this comment

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

git log --follow programs/test/benchmark.c tracks the file through delete/recreate.

@valeriosetti valeriosetti added the needs-preceding-pr Requires another PR to be merged first label Feb 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-preceding-pr Requires another PR to be merged first 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 priority-high High priority - will be reviewed soon size-s Estimated task size: small (~2d)
Projects
Development

Successfully merging this pull request may close these issues.

Remove DHM module
4 participants