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 ...
|
thetek42
force-pushed
the
async-tls-server
branch
from
33a905d
to
07ff5af
Compare
github-actions
bot
changed the title
There was a problem hiding this comment.
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.
espressif-bot
added
Status: In Progress
|
@AdityaHPatwardhan I updated the function names and also ported them to both WolfSSL and esp-tls. |
| @@ -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.
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.
Sorry for the misunderstanding, but can you please clarify what your suggestion here is?
There was a problem hiding this comment.
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.
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.
| * @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. |
thetek42
force-pushed
the
async-tls-server
branch
from
68aa450
to
c06b501
Compare
thetek42
force-pushed
the
async-tls-server
branch
from
c06b501
to
083b396
Compare
|
PR updated. Thanks! |
|
sha=083b39640524dc96d5bccffc396667f5f3a616f3 |
This makes
esp_mbedtls_server_session_createasync-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)