[PATCH 1 of 4] Core: added socket protocol

[Maxim Dounin]

Hello!

On Thu, May 16, 2024 at 02:38:53PM +0200, maxime wrote:

> # HG changeset patch
> # User maxime <mux99 at live.be>
> # Date 1715588655 -7200
> #      Mon May 13 10:24:15 2024 +0200
> # Node ID dcadf0a3d97ff4d677440060290e9cda84c69ac7
> # Parent  3d455e37abf870f79be26c36d6b1d9cad2c4dd16
> Core: added socket protocol.
> 
> This patch updates the creation of listening sockets to use a new field
> of the `ngx_listening_s` structure. The `protocol` field can be used in
> conjunction with the `type` to specify the protocol to be used.
> 
> Modules will then be able to specify a different protocol, e.g.
> IPPROTO_MPTCP.
> 
> diff --git a/src/core/ngx_connection.c b/src/core/ngx_connection.c
> --- a/src/core/ngx_connection.c
> +++ b/src/core/ngx_connection.c
> @@ -487,7 +487,18 @@ ngx_open_listening_sockets(ngx_cycle_t *
>                  continue;
>              }
>  
> -            s = ngx_socket(ls[i].sockaddr->sa_family, ls[i].type, 0);
> +            s = (ngx_socket_t) -1;
> +            if (ls[i].protocol > 0) {
> +                s = ngx_socket(ls[i].sockaddr->sa_family, ls[i].type,
> +                               ls[i].protocol);
> +                /* In case of error, retry with the default protocol */
> +                ngx_log_error(NGX_LOG_NOTICE, log, 0,
> +                      "socket(%d) failed, trying with 0", ls[i].protocol);
> +            }
> +
> +            if (s == (ngx_socket_t) -1) {
> +                s = ngx_socket(ls[i].sockaddr->sa_family, ls[i].type, 0);
> +            }
>  
>              if (s == (ngx_socket_t) -1) {
>                  ngx_log_error(NGX_LOG_EMERG, log, ngx_socket_errno,

[...]

Thanks for the patches.

Some comments, in no particular order, and not limited to the 
particular patch:

- Retrying with the default protocol seems to be specific for 
  Multipath TCP, which isn't really a separate protocol, but 
  rather a TCP extension.  And should be limited only to 
  Multipath TCP, if done at all.

  I'm not sure if it needs to be here at all though: as long as 
  Multipath TCP is explicitly requested in the configuration, it 
  might not be a good idea to continue if we aren't able to create 
  such socket.

- Generic non-default protocol support will need protocol-specific 
  matching in various places, as well as retrieving the protocol 
  from inherited sockets.  E.g., SCTP sockets can coexist with 
  TCP sockets listening on the same address/port pair.

- I certainly do not like the idea of 

#ifndef IPPROTO_MPTCP
#define IPPROTO_MPTCP 262
#endif

  Especially when it is done in each module which creates listen 
  sockets.  Similarly, I don't really like the idea of making this 
  Linux-specific.

  Rather, #ifdef IPPROTO_MPTCP might be a good way to restrict 
  things to systems which do support creation of Multipath TCP 
  listening sockets via socket(IPPROTO_MPTCP).

  It looks like IPPROTO_MPTCP define is readily available on 
  modern Linux systems, and can be safely used.  For example, it is 
  present on Ubuntu 22.04 LTS and later version, so the only 
  supported Ubuntu version where it is not available is 20.04.  It 
  is also available on both Rocky Linux 8 and Rocky Linux 9, all 
  supported Alpine Linux versions (3.16 and up), Debian 11 and up 
  (not available only in Debian 10).

- I tend to think that "multipath" might be a better name for the 
  "listen" directive parameter.

Overall, I rather see it as a "listen ... multipath" at the 
configuration level, and a listening socket flag "multipath" which 
is then handled by ngx_open_listening_sockets() as appropriate for 
a particular platform (currently via socket(IPPROTO_MPTCP) on 
Linux).

Hope this helps.

-- 
Maxim Dounin
http://mdounin.ru/