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

Do not add error codes #9926

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

Do not add error codes #9926

wants to merge 6 commits into from

Conversation

gilles-peskine-arm
Copy link
Contributor

Never add a low-level error code to a high-level error code. Just use the low-level error code. Resolves #9619, a step towards having unified PSA and mbedtls error codes.

PR checklist

Please remove the segment/s on either side of the | symbol as appropriate, and add any relevant link/s to the end of the line.
If the provided content is part of the present PR remove the # symbol.

  • changelog provided
  • development PR here
  • crypto PR Do not add error codes TF-PSA-Crypto#167
  • framework PR not required
  • 3.6 PR not required because: API break
  • 2.28 PR not required because: API break
  • tests provided | not required because:

@gilles-peskine-arm
Don't expect added error codes
03190d9 
Signed-off-by: Gilles Peskine <[email protected]>
@gilles-peskine-arm gilles-peskine-arm added component-platform Portability layer and build scripts needs-ci Needs to pass CI tests size-s Estimated task size: small (~2d) priority-high High priority - will be reviewed soon labels Jan 24, 2025
This was referenced Jan 24, 2025
Merged
Open
Closed
@gilles-peskine-arm
Changelog entry for error code space unification
23b3593 
Signed-off-by: Gilles Peskine <[email protected]>
@gilles-peskine-arm
Update crypto submodule
11df011 
Signed-off-by: Gilles Peskine <[email protected]>
@gilles-peskine-arm
Remove mbedtls_low_level_sterr() and mbedtls_high_level_strerr()
87714db 
Just removed from the API. We can greatly simplify error.c but that will be
for later.

Signed-off-by: Gilles Peskine <[email protected]>
@gilles-peskine-arm gilles-peskine-arm force-pushed the error-codes-add-force-low-mbedtls branch from a1c6c72 to 87714db Compare January 24, 2025 17:26
@gilles-peskine-arm gilles-peskine-arm 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 Jan 24, 2025
@minosgalanakis minosgalanakis self-requested a review January 27, 2025 10:02
@mpg mpg self-requested a review January 27, 2025 11:30
@mpg mpg removed the needs-reviewer This PR needs someone to pick it up for review label Jan 27, 2025
mpg
mpg reviewed Jan 27, 2025
View reviewed changes
Copy link
Contributor

@mpg mpg left a comment

Choose a reason for hiding this comment

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

LGTM except the bit in the ChangeLog I already pointed out in the other PR.

Regarding the last commit's message: do we already have an issue to track further simplifications?

@gilles-peskine-arm
Copy link
Contributor Author

Regarding the last commit's message: do we already have an issue to track further simplifications?

Not yet, that may come from task breakdown on Mbed-TLS/TF-PSA-Crypto#145 but at this point it's not clear to me yet whether that's high enough priority to make the cut.

@minosgalanakis
Copy link
Contributor

Looks good, but will need to be rebased when the TF-PSA-Crypto comment's are addressed.

mpg
mpg previously approved these changes Jan 31, 2025
View reviewed changes
Copy link
Contributor

@mpg mpg left a comment

Choose a reason for hiding this comment

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

LGTM

minosgalanakis
minosgalanakis previously approved these changes Feb 3, 2025
View reviewed changes
Copy link
Contributor

@minosgalanakis minosgalanakis left a comment

Choose a reason for hiding this comment

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

LGTM but will also need a rebase

@mpg
Copy link
Contributor

mpg commented Feb 20, 2025

We need to update the tf-psa-crypto submodule pointer once Mbed-TLS/TF-PSA-Crypto#167 has made it through the merge queue.

@mpg mpg added needs-work approved Design and code approved - may be waiting for CI or backports needs-preceding-pr Requires another PR to be merged first and removed needs-review Every commit must be reviewed by at least two team members, needs-work labels Feb 20, 2025
@gilles-peskine-arm
Merge remote-tracking branch 'development' into error-codes-add-force…
b014043 
…-low-mbedtls

Update the tf-psa-crypto submodule to the head of its 'development' branch.
@gilles-peskine-arm gilles-peskine-arm added needs-review Every commit must be reviewed by at least two team members, and removed needs-preceding-pr Requires another PR to be merged first approved Design and code approved - may be waiting for CI or backports labels Feb 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mpg mpg Awaiting requested review from mpg

@minosgalanakis minosgalanakis Awaiting requested review from minosgalanakis

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
component-platform Portability layer and build scripts needs-review Every commit must be reviewed by at least two team members, priority-high High priority - will be reviewed soon size-s Estimated task size: small (~2d)
Projects
Roadmap pull requests (new board)
Status: Has Approval
Roadmap Board for Mbed TLS
Status: To Do
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Error codes in 4.0
3 participants
@gilles-peskine-arm @minosgalanakis @mpg