[PATCH 1 of 4] Core: added socket protocol

Wed May 22 15:03:22 UTC 2024

 Thank you for your reply! Your comments make sense, I will look at
 sending a new version later. But this might take a bit more time as
 I've important deadlines coming soon.

________________________________________
De : Maxim Dounin <mdounin at mdounin.ru>
Envoyé : mercredi 22 mai 2024 13:29
À : nginx-devel at freenginx.org <nginx-devel at freenginx.org>
Cc : mux99 at live.be <mux99 at live.be>
Objet : Re: [PATCH 1 of 4] Core: added socket protocol

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/

________________________________________
De : Maxim Dounin <mdounin at mdounin.ru>
Envoyé : mercredi 22 mai 2024 13:29
À : nginx-devel at freenginx.org <nginx-devel at freenginx.org>
Cc : mux99 at live.be <mux99 at live.be>
Objet : Re: [PATCH 1 of 4] Core: added socket protocol