cesanta / mongoose

Embedded Web Server
https://mongoose.ws
Other
11.13k stars 2.73k forks source link

fn connect()'s return value is wrong in mg_connect_resolved(). #1994

Closed tercelcn closed 1 year ago

tercelcn commented 1 year ago

connect() ' return value is BSD_EINPROGRESS instead of BSD_EWOULDBLOCK.

#if defined(MG_ENABLE_RL) && MG_ENABLE_RL
rc = connect(FD(c), &usa.sa, slen); 
if (( rc == 0 ) || ( rc == EISCONN )) {
      mg_call(c, MG_EV_CONNECT, NULL);
    } else if (( rc == EINPROGRESS ) || ( rc == EALREADY )) {
#else
    if ((rc = connect(FD(c), &usa.sa, slen)) == 0) {
      mg_call(c, MG_EV_CONNECT, NULL);
    } else if (mg_sock_would_block()) {
#endif

Environment

scaprile commented 1 year ago

Does your issue mean that we are expecting a response code from RTX that is incorrect and your code above is the fix to that issue ? Can you point to the docs that can confirm that, please ? Otherwise, please explain

tercelcn commented 1 year ago

Yes, my code above fix it. please read manual for RLNET. https://www.keil.com/pack/doc/mw/Network/html/group__using__network__sockets__bsd__func.html#ga2527bb1268c12185a1a0ab8b1c5a11d0

scaprile commented 1 year ago

The functions in that documentation do not use errno, while Mongoose is expecting them to fill that variable. Your patch is a workaround for the tip of the iceberg. Did you define MG_ARCH=MG_ARCH_RTX ? Do you get compile errors ?

tercelcn commented 1 year ago

I define MG_ARCH=MG_ARCH_RTX, and no errors.

cpq commented 1 year ago

Yeah, the RL NET API, whilst it looks like a socket API, is not a standard socket API. The "true" socket API must return -1 or errors, and keep the error in the errno variable. RL NET, however, returns error straight in the error code. Which is, in my opinion, is better, but it is non-standard.

So we should reorganise the code that looks like this (which does not work with RL NET):

  int result = some_socket_api_function(....);
  if (result < 0 && mg_sock_would_block()) ....;

Into the code that works with both usual socket implementation, and RL NET implementation. Maybe we must adopt RL NET style ?

  int result = some_socket_api_function(....);
  if (result < 0) result = -MG_SOCKET_ERRNO;  // RL NET must define MG_SOCKET_ERRNO as -result
  if (mg_sock_would_block(result)) ....;
scaprile commented 1 year ago

Good idea, what about:

  int result = some_socket_api_function(....);
#if defined(MG_ENABLE_RL) && MG_ENABLE_RL 
  if (result < 0) MG_SOCKET_ERRNO = result;
#endif

this will mean minimum changes to the current implementation, hence minimum possible hassle on what is already working,

Yeah, the RLNET way is cooler but non-standard The OP reports no compilation errors so they provide an header, but don't use it.

cpq commented 1 year ago

@tercelcn I have pushed https://github.com/cesanta/mongoose/tree/rl

Please grab https://raw.githubusercontent.com/cesanta/mongoose/rl/mongoose.h and https://raw.githubusercontent.com/cesanta/mongoose/rl/mongoose.c , copy to your source tree and see how that works.

About your suggestion: if (( rc == 0 ) || ( rc == EISCONN )) { I don't think we need to check against the EISCONN. I believe EISCONN is for the cases when we have an already connected socket, and attempt to connect() it again. Mongoose does not do that.

@scaprile I wish we could build a dashboard example on Github Actions automatically using Keil compiler, and test the firmware on a real hardware like we do for GCC compiler. I wish that quite strong. This would make sure our customers who use Keil, have the same level of service as those who use GCC - let's investigate that offline

@tercelcn could you elaborate a little what company/product you work on, please? You could email to support@cesanta.com if that is sensitive for the public github.

tercelcn commented 1 year ago

@cpq the source patched work correctly. I am looking for an MQTT broker to work on MCU, such as STM32F767, now I'm evaluating mongoose.