-
Notifications
You must be signed in to change notification settings - Fork 7.4k
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
Make esp_mbedtls_server_session_create async compatible (IDFGH-13606) #14493
Conversation
👋 Hello thetek42, we appreciate your contribution to this project! 📘 Please review the project's Contributions Guide for key guidelines on code, documentation, testing, and more. 🖊️ Please also make sure you have read and signed the Contributor License Agreement for this project. Click to see more instructions ...
Review and merge process you can expect ...
|
33a905d
to
07ff5af
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR @thetek42,
I agree that an async version would be helpful.
So the mbedtls is an internal layer for esp-tls.
It is not supposed to be directly used publicly (outside of esp-tls component)
Instead I think we should create appropriate APIs for the esp-tls layer and use that layer publicly.
@AdityaHPatwardhan I updated the function names and also ported them to both WolfSSL and esp-tls. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for updating the PR.
Just a couple of suggestions, otherwise looks good to me
@@ -90,6 +92,8 @@ static const char *TAG = "esp-tls"; | |||
#define _esp_tls_conn_delete esp_wolfssl_conn_delete | |||
#define _esp_tls_net_init esp_wolfssl_net_init | |||
#define _esp_tls_server_session_create esp_wolfssl_server_session_create | |||
#define _esp_tls_server_session_init esp_wolfssl_server_session_init |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can keep them as NULL
and then handle it esp_tls_server_session_init
down below by returning failure in the API that is calling this function when it is NULL
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the misunderstanding, but can you please clarify what your suggestion here is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant that the support for wolfSSL need not be added. Since I am not sure if you have tested the wolfSSL support yourself and we dont require wolfSSL support to be added so we can just mark the _esp_tls_server_session_create
as NULL
.
and in the esp_tls_server_session_create
API where you use _esp_tls_server_session_create
you can just handle this case by doing something like
if (_esp_tls_server_session_create == NULL) {
return ESP_ERR_NOT_SUPPORTED.
}
/** | ||
* @brief Initialization part of esp_tls_server_session_create | ||
*/ | ||
int esp_tls_server_session_init(esp_tls_cfg_server_t *cfg, int sockfd, esp_tls_t *tls) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding the new API.
Is it possible to keep the return type as esp_err_t in this case.
I think this would go well with the other APIs added in esp_tls.h
.
Sorry for the late suggestion, I can also do this in a supplementary commit.
return _esp_tls_server_session_init(cfg, sockfd, tls); | ||
} | ||
/** | ||
* @brief Asynchronous continue of esp_tls_server_session_create, to be |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* @brief Asynchronous continue of esp_tls_server_session_create, to be | |
* @brief Asynchronous continue of esp_tls_server_session_create, to be | |
* called in a loop by the user until it returns 0. If this functions returns something other than 0, ESP_TLS_ERR_SSL_WANT_READ or ESP_TLS_ERR_SSL_WANT_WRITE, the esp-tls context must not be used and should be freed using esp_tls_conn_destroy(); | |
@thetek42 Actually, if it is fine with you, I can take over the PR. Please squash your changes in one commit. I will update my changes on a separate commit so your contribution would still be visible. |
68aa450
to
c06b501
Compare
c06b501
to
083b396
Compare
PR updated. Thanks! |
sha=083b39640524dc96d5bccffc396667f5f3a616f3 |
This makes
esp_mbedtls_server_session_create
async-friendly by splitting up the function into two parts. This change was originally proposed here in order to make an asynchronous TLS server possible: esp-rs/esp-idf-svc#368 (comment)