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

Introduce mbedtls_ssl_hs_cb_t typedef #5623

Merged
merged 1 commit into from
Apr 8, 2022

Conversation

gstrauss
Copy link
Contributor

Description

  • Introduce mbedtls_ssl_hs_cb_t typedef
  • Inline func for mbedtls_ssl_conf_cert_cb()

Follow-up PR from discussion with @mpg in #5454.
#5454 (comment)
#5454 (comment)
This PR aims to slightly improve the interface. This PR is lower priority as this is not a fix to anything.

Related question: should all new interfaces which are simple getter/setter functions be inlined in the header when struct definition is visible, even if the inline func uses MBEDTLS_PRIVATE? (e.g. setter/getter for mbedtls_ssl_config and mbedtls_ssl_context are available, but struct mbedtls_ssl_handshake_params (library/ssl_misc.h) is not visible.) Note: This should not be done for older interfaces (until possibly after a major version bump) since compiled application code might hold references to the symbols.

Status

READY

Requires Backporting

NO

Migrations

NO

Todos

  • Documentation
  • Changelog updated

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.

This generally looks good to me, just a couple of niggles about the changelog.

@@ -0,0 +1,3 @@
Features
* Introduce mbedtls_ssl_hs_cb_t typedef.
Copy link
Member

Choose a reason for hiding this comment

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

It would be useful for the reader, I think, to know what this typedef is used for as well as what it is called.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🚀

@@ -0,0 +1,3 @@
Features
* Introduce mbedtls_ssl_hs_cb_t typedef.
* Inline func for mbedtls_ssl_conf_cert_cb().
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is major enough to warrant an entry in the changelog.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🚀

Inline func for mbedtls_ssl_conf_cert_cb()

Signed-off-by: Glenn Strauss <[email protected]>
Copy link
Contributor

@tom-cosgrove-arm tom-cosgrove-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

@paul-elliott-arm
Copy link
Member

ABI-API is expected, pr-merge failing is OpenCI Failure.

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

Roadmap Board for Mbed TLS automation moved this from In Review to Has Approval Apr 8, 2022
@paul-elliott-arm paul-elliott-arm removed the needs-review Every commit must be reviewed by at least two team members, label Apr 8, 2022
@paul-elliott-arm paul-elliott-arm added the approved Design and code approved - may be waiting for CI or backports label Apr 8, 2022
@paul-elliott-arm paul-elliott-arm merged commit ed334d2 into Mbed-TLS:development Apr 8, 2022
Roadmap Board for Mbed TLS automation moved this from Has Approval to Done Apr 8, 2022
@daverodgman daverodgman added this to Mbed TLS 3.x in Backlog for Mbed TLS Feb 22, 2023
@daverodgman daverodgman removed this from Mbed TLS 3.x in EPICs for Mbed TLS Feb 22, 2023
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 size-s Estimated task size: small (~2d)
Projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants