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

@minosgalanakis minosgalanakis added Community size-s Estimated task size: small (~2d) labels Mar 14, 2022
@minosgalanakis minosgalanakis 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 Product Backlog labels Mar 14, 2022
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.

🚀

@AndrzejKurek AndrzejKurek removed the needs-reviewer This PR needs someone to pick it up for review label Apr 6, 2022
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

@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
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
None yet
Development

Successfully merging this pull request may close these issues.

5 participants