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

Error codes: enforce add macro #9566

Conversation

gilles-peskine-arm
Copy link
Contributor

Enforce the use of MBEDTLS_ERROR_ADD to add a low-level error code with a high-level error code.

This is a step of DI towards.#8501.

PR checklist

  • changelog not required because: no behavior change
  • development PR here
  • framework PR not required
  • 3.6 PR not required because: 4.0 prep
  • 2.28 PR not required because: 4.0 prep
  • tests provided

@gilles-peskine-arm gilles-peskine-arm added needs-ci Needs to pass CI tests component-test Test framework and CI scripts priority-high High priority - will be reviewed soon size-xs Estimated task size: extra small (a few hours at most) 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 Sep 15, 2024
@yanesca yanesca self-requested a review September 16, 2024 10:58
yanesca
yanesca previously approved these changes Sep 16, 2024
Copy link
Contributor

@yanesca yanesca left a comment

Choose a reason for hiding this comment

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

I have reviewed the script in the commit message and executed it locally arriving at the same result. I have reviewed the last two commits manually.

Looks good to me.

Copy link
Member

@paul-elliott-arm paul-elliott-arm left a comment

Choose a reason for hiding this comment

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

LGTM, however will need rebase due to conflicts.

Replace obvious additions of an `MBEDTLS_ERR_xxx` constant by a call to
`MBEDTLS_ERROR_ADD`.

Skip `case` statements since `MBEDTLS_ERROR_ADD(pp_constant)` is not a
preprocessor constant.

This commit does not replace additions split over lines. Those will be
handled in a subsequent commit.

```
git ls-files '*.h' '*.c' '*.function' '*.data' |
xargs perl -i -pe '
    next if /\bcase\b/;
    s/\b(MBEDTLS_ERR_\w+)\s*\+\s*(\w+)\b/MBEDTLS_ERROR_ADD($1, $2)/g;
    s/\b(\w+)\s*\+\s*(MBEDTLS_ERR_\w+)\b/MBEDTLS_ERROR_ADD($1, $2)/g'
```

Signed-off-by: Gilles Peskine <[email protected]>
Reject direct additions of error constants (regex-based approximation).

Fix the lone straggler.

Signed-off-by: Gilles Peskine <[email protected]>
@gilles-peskine-arm
Copy link
Contributor Author

I have rebased on top of the latest development to take care of the merge conflict. For "Use MBEDTLS_ERROR_ADD instead of explicit addition: simple cases", I replayed the script. For the other commits, there was no conflict.

Copy link
Member

@paul-elliott-arm paul-elliott-arm left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@yanesca yanesca left a comment

Choose a reason for hiding this comment

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

LGTM

@yanesca yanesca added this pull request to the merge queue Oct 16, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 16, 2024
@gilles-peskine-arm
Copy link
Contributor Author

The failure on the merge queue was a timeout in one test case of ssl-opt.sh. I'm requeuing it.

@gilles-peskine-arm gilles-peskine-arm added this pull request to the merge queue Oct 16, 2024
@gilles-peskine-arm gilles-peskine-arm 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 Oct 16, 2024
@gilles-peskine-arm gilles-peskine-arm added the approved Design and code approved - may be waiting for CI or backports label Oct 16, 2024
Merged via the queue into Mbed-TLS:development with commit e1f37c5 Oct 16, 2024
4 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Design and code approved - may be waiting for CI or backports component-test Test framework and CI scripts priority-high High priority - will be reviewed soon size-xs Estimated task size: extra small (a few hours at most)
Development

Successfully merging this pull request may close these issues.

3 participants