The code appears functional but has some potential issues that can be improved.
Error handling
While error handling is done in some cases, such as checking for null pointers and handling failed SSL functions, there are other parts where error handling can be improved. For instance, when calling socket(), connect(), SSL_write(), and SSL_read(), the return values should be checked for errors and appropriate messages should be logged or propagated.
Resource leaks
In cases of unhandled errors, resources allocated to 'Addr' may not be freed, leading to memory leaks. The code should ensure freeaddrinfo(Addrs) is called in all cases, including on error conditions.
Missing include files
Some functions such as spdlog::error(), getaddrinfo(), and ERR_print_errors_fp() may not have been declared or included in the provided code snippet. Ensure you have the required include files for these functions.
Buffer size
Currently, the buffer size for SSL_read() is hardcoded to 1024. It would be better to declare this as a constant or allow the user to provide a buffer size.
Namespace
It would be useful to add comments about the purpose of the WasmEdge and Host namespaces.
Function naming
Function naming can be inconsistent. For example, WasmEdgeHttpsReqGetRcvLen is using camel case, while other functions like WasmEdgeHttpsReqSendData are not. Consider adhering to a consistent naming convention.
Overall, the code appears functional but can benefit from improved error handling, cleanup, and consistency.
The patch introduces the following key changes:
Addition of an include statement for the openssl/tls1.h header file.
#include <openssl/tls1.h>
Addition of a call to SSL_set_tlsext_host_name() function before the SSL_connect() call.
SSL_set_tlsext_host_name(Ssl, Host);
Improved error reporting when SSL_connect() fails by introducing a variable 'Code' to store the returned error code from SSL_get_error() function, and changing the logging message to log the error code.
juntao
commented
1 year ago
Hello, I am a serverless review bot on flows.network. Here are my reviews of changed source code files in this PR.
plugins/wasmedge_httpsreq/httpsreqfunc.cpp
The code appears functional but has some potential issues that can be improved.
Error handling While error handling is done in some cases, such as checking for null pointers and handling failed SSL functions, there are other parts where error handling can be improved. For instance, when calling socket(), connect(), SSL_write(), and SSL_read(), the return values should be checked for errors and appropriate messages should be logged or propagated.
Resource leaks In cases of unhandled errors, resources allocated to 'Addr' may not be freed, leading to memory leaks. The code should ensure freeaddrinfo(Addrs) is called in all cases, including on error conditions.
Missing include files Some functions such as spdlog::error(), getaddrinfo(), and ERR_print_errors_fp() may not have been declared or included in the provided code snippet. Ensure you have the required include files for these functions.
Buffer size Currently, the buffer size for SSL_read() is hardcoded to 1024. It would be better to declare this as a constant or allow the user to provide a buffer size.
Namespace It would be useful to add comments about the purpose of the WasmEdge and Host namespaces.
Function naming Function naming can be inconsistent. For example, WasmEdgeHttpsReqGetRcvLen is using camel case, while other functions like WasmEdgeHttpsReqSendData are not. Consider adhering to a consistent naming convention.
Overall, the code appears functional but can benefit from improved error handling, cleanup, and consistency.
The patch introduces the following key changes:
Addition of an include statement for the openssl/tls1.h header file.
Addition of a call to SSL_set_tlsext_host_name() function before the SSL_connect() call.
Improved error reporting when SSL_connect() fails by introducing a variable 'Code' to store the returned error code from SSL_get_error() function, and changing the logging message to log the error code.
cc https://github.com/WasmEdge/WasmEdge/pull/2425