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 <mdou...@mdounin.ru>
Envoyé : mercredi 22 mai 2024 13:29
À : nginx-devel@freenginx.org <nginx-devel@freenginx.org>
Cc : mu...@live.be <mu...@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 <mu...@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 <mdou...@mdounin.ru>
Envoyé : mercredi 22 mai 2024 13:29
À : nginx-devel@freenginx.org <nginx-devel@freenginx.org>
Cc : mu...@live.be <mu...@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 <mu...@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/
--
nginx-devel mailing list
nginx-devel@freenginx.org
https://freenginx.org/mailman/listinfo/nginx-devel
