mosquitto

Why 2 layers of loops inside mqtt3_socket_accept() when initializing SSL related resources?

Asked by Yun Wu

Dear sir,

In mqtt3_socket_accept(),

#ifdef WITH_TLS
  /* TLS init */
  for(i=0; i<db->config->listener_count; i++){
   for(j=0; j<db->config->listeners[i].sock_count; j++){
    if(db->config->listeners[i].socks[j] == listensock){
     if(db->config->listeners[i].ssl_ctx){
      new_context->ssl = SSL_new(db->config->listeners[i].ssl_ctx);
      if(!new_context->ssl){
       mqtt3_context_cleanup(NULL, new_context, true);
       return -1;
      }
      SSL_set_ex_data(new_context->ssl, tls_ex_index_context, new_context);
      SSL_set_ex_data(new_context->ssl, tls_ex_index_listener, &db->config->listeners[i]);
      new_context->want_read = true;
      new_context->want_write = true;
      bio = BIO_new_socket(new_sock, BIO_NOCLOSE);
      SSL_set_bio(new_context->ssl, bio, bio);
      rc = SSL_accept(new_context->ssl);
      if(rc != 1){
       rc = SSL_get_error(new_context->ssl, rc);
       if(rc == SSL_ERROR_WANT_READ){
        new_context->want_read = true;
       }else if(rc == SSL_ERROR_WANT_WRITE){
        new_context->want_write = true;
       }else{
        e = ERR_get_error();
        while(e){
         _mosquitto_log_printf(NULL, MOSQ_LOG_NOTICE,
           "Client connection from %s failed: %s.",
           new_context->address, ERR_error_string(e, ebuf));
         e = ERR_get_error();
        }
        |<---- Question 3): Why not return error here?
       }
       |<---- Question 2): Why no jump out of the 2 layers of loops here;?
      }
     }
    }
   }
  }
#endif

I just think these codes are initializing SSL related resources.
So my question is:

1)

why 2 layers of loops are used here?
Is the purpose to find its listener structure?
But it is already found here:

  for(i=0; i<db->config->listener_count; i++){
   for(j=0; j<db->config->listeners[i].sock_count; j++){
    if(db->config->listeners[i].socks[j] == listensock){
     new_context->listener = &db->config->listeners[i];
     new_context->listener->client_count++;
     break;
    }
   }
  }
  if(!new_context->listener){
   mqtt3_context_cleanup(NULL, new_context, true);
   return -1;
  }

2) Why no break to jump out of the 2 layers of loop after SSL related resource initialized?

3) Why not return error when error occurs?

4) Can we just pass pointer to struct listener instead of listensock to mqtt3_socket_accept() so we don't have to walk db->config->listeners to get the listener structure bound to the accepted new socket?

Thanks.

Question information

Language:
English Edit question
Status:
Solved
For:
mosquitto Edit question
Assignee:
No assignee Edit question
Solved by:
Yun Wu
Solved:
Last query:
Last reply:
Revision history for this message
Roger Light (roger.light) said : 		#1

Thanks for your comments.

1. I agree, this shouldn't be duplicated. It isn't intentional. As it is, the overhead in most cases will be pretty trivial but it is good to fix.

2. Likewise, but once the loops are gone this won't matter.

3. A more recent version of the code than you are looking already does return an error there.

4. Unfortunately not - we only know the fd number of the listening socket when accept is called. We need to walk the listeners array to find which struct it relates to. We could pass pointer to the struct, but we'd just have to do the walking outside the function so it makes little difference. In most cases there will be only one listener anyway I would imagine.

Revision history for this message
Yun Wu (wuyun1984-1984) said : 		#2

OK, thanks.

4.
In fact you can.
You can move listensock array and listensock_count generating from main() to mosquitto_main_loop().
Then you can directly pass pointer to struct listener to mqtt3_socket_accept() :-)

BTW, I just think it would be more logic to get all sockets inside mosquitto_main_loop() and then enter while () loop.
Because in current codes, when I added sockets in mosquitto, I had to modify prototype mosquitto_main_loop() again and again.