File 6248-Rewrite-the-listen_port_options-1-test-cas... of Package erlang

Overview Repositories Revisions Requests Users Attributes Meta

File 6248-Rewrite-the-listen_port_options-1-test-case.patch of Package erlang (Revision 6cc4e2769dd7ada0d41c527b51da0114)

Currently displaying revision 6cc4e2769dd7ada0d41c527b51da0114 , Show latest

From 6d98be254b6dedef741c6ba7ab832718e04fb56b Mon Sep 17 00:00:00 2001
From: Raimo Niskanen <raimo@erlang.org>
Date: Mon, 3 Oct 2022 17:14:32 +0200
Subject: [PATCH 08/27] Rewrite the listen_port_options/1 test case

The test most probably did not test what was intended.
Since {reuseaddr,true} is default for the distribution
listen socket the test relied on the Unix semantics of not
allowing a second listening socket on the same port while
there exists an active listening socket on that port.

That semantics differ on Windows where {reuseaddr,true}
apparently allows creating a duplicate active listening socket.

The new test instead creates a connection and closes the
listening socket.  Now the server port can be reused
if {reuseaddr,true} is applied, which is the default.

Instead of trying to use an occupied port and verify
that it fails (which did not work on Windows), create
a connection and check by inspecting the result of
net_kernel:node_info/1 that the options for setting
listen port managed to do their thing.
---
 lib/kernel/src/inet_tcp_dist.erl |  71 +++++++++----
 lib/ssl/test/ssl_dist_SUITE.erl  | 169 ++++++++++---------------------
 2 files changed, 105 insertions(+), 135 deletions(-)

diff --git a/lib/kernel/src/inet_tcp_dist.erl b/lib/kernel/src/inet_tcp_dist.erl
index b6a0cec0ab..b53de6281b 100644
--- a/lib/kernel/src/inet_tcp_dist.erl
+++ b/lib/kernel/src/inet_tcp_dist.erl
@@ -86,7 +86,7 @@ listen(Name) ->
 
 gen_listen(Driver, Name, Host) ->
     ErlEpmd = net_kernel:epmd_module(),
-    case gen_listen(ErlEpmd, Name, Host, Driver, [{active, false}, {packet,2}, {reuseaddr, true}]) of
+    case gen_listen(ErlEpmd, Name, Host, Driver) of
 	{ok, Socket} ->
 	    TcpAddress = get_tcp_address(Driver, Socket),
 	    {_,Port} = TcpAddress#net_address.address,
@@ -100,8 +100,8 @@ gen_listen(Driver, Name, Host) ->
 	    Error
     end.
 
-gen_listen(ErlEpmd, Name, Host, Driver, Options) ->
-    ListenOptions = listen_options(Options),
+gen_listen(ErlEpmd, Name, Host, Driver) ->
+    ListenOptions = listen_options(),
     case call_epmd_function(ErlEpmd, listen_port_please, [Name, Host]) of
         {ok, 0} ->
             {First,Last} = get_port_range(),
@@ -133,25 +133,52 @@ do_listen(Driver, First,Last,Options) ->
 	    Other
     end.
 
-listen_options(Opts0) ->
-    Opts1 =
-	case application:get_env(kernel, inet_dist_use_interface) of
-	    {ok, Ip} ->
-		[{ip, Ip} | Opts0];
-	    _ ->
-		Opts0
-	end,
-    case application:get_env(kernel, inet_dist_listen_options) of
-	{ok,ListenOpts} ->
-	    case proplists:is_defined(backlog, ListenOpts) of
-		true ->
-		    ListenOpts ++ Opts1;
-		false ->
-		    ListenOpts ++ [{backlog, 128} | Opts1]
-	    end;
-	_ ->
-	    [{backlog, 128} | Opts1]
-    end.
+listen_options() ->
+    DefaultOpts = [{reuseaddr, true}, {backlog, 128}],
+    ForcedOpts =
+        [{active, false}, {packet,2} |
+         case application:get_env(kernel, inet_dist_use_interface) of
+             {ok, Ip}  -> [{ip, Ip}];
+             undefined -> []
+         end],
+    Force = maps:from_list(ForcedOpts),
+    InetDistListenOpts =
+        case application:get_env(kernel, inet_dist_listen_options) of
+            {ok, Opts} -> Opts;
+            undefined  -> []
+        end,
+    ListenOpts = listen_options(InetDistListenOpts, ForcedOpts, Force),
+    Seen =
+        maps:from_list(
+          lists:filter(
+            fun ({_,_}) -> true;
+                (_)     -> false
+            end, ListenOpts)),
+    lists:filter(
+      fun ({OptName,_}) when is_map_key(OptName, Seen) ->
+              false;
+          (_) ->
+              true
+      end, DefaultOpts) ++ ListenOpts.
+
+%% Pass through all but forced
+listen_options([Opt | Opts], ForcedOpts, Force) ->
+    case Opt of
+        {OptName,_} ->
+            case is_map_key(OptName, Force) of
+                true ->
+                    listen_options(Opts, ForcedOpts, Force);
+                false ->
+                    [Opt |
+                     listen_options(Opts, ForcedOpts, Force)]
+            end;
+        _ ->
+            [Opt |
+             listen_options(Opts, ForcedOpts, Force)]
+    end;
+listen_options([], ForcedOpts, _Force) ->
+    %% Append forced
+    ForcedOpts.
 
 
 %% ------------------------------------------------------------
diff --git a/lib/ssl/test/ssl_dist_SUITE.erl b/lib/ssl/test/ssl_dist_SUITE.erl
index f7b3264d37..c6154855e5 100644
--- a/lib/ssl/test/ssl_dist_SUITE.erl
+++ b/lib/ssl/test/ssl_dist_SUITE.erl
@@ -281,88 +282,45 @@ nodelay_option(Config) ->
 listen_port_options() ->
     [{doc, "Test specifying listening ports"}].
 listen_port_options(Config) when is_list(Config) ->
-    %% Start a node, and get the port number it's listening on.
+    %% Set up the probably most supported scenario
+    %% for {reuseaddr,true}, i.e, the listening socket
+    %% is closed, but an accepted server side socket
+    %% blocks the server port, unless {reuseaddr,true}
+    %% is used.
+    %%
+    %% Set up a server socket and close the listening socket
+    {ok, L}    = gen_tcp:listen(0, [{reuseaddr,true}]),
+    {ok, Port} = inet:port(L),
+    {ok, C}    = gen_tcp:connect({127,0,0,1}, Port, []),
+    {ok, S}    = gen_tcp:accept(L),
+    ok         = gen_tcp:close(L),
+    ct:pal("Port: ~w", [Port]),
+    %%
+    %% Start a node on the server port, {reuseaddr,true}
+    %% is used per default on the listening socket
+    %% since it is a server - see inet_tcp_dist:gen_listen/3
+    PortOpts =
+        "-kernel"
+        " inet_dist_listen_min " ++ integer_to_list(Port) ++
+        " inet_dist_listen_max " ++ integer_to_list(Port),
+    %% basic_test/3 connects NH1 -> NH2 so it is NH2 that should
+    %% act as server to make use of PortOpts
     NH1 = start_ssl_node(Config),
-    NH1Args = apply_on_ssl_node(NH1, init, get_arguments, []),
-    {ok, NH1NodesPorts} = apply_on_ssl_node(NH1, net_adm, names, []),
-    io:format(
-      "Node 1 Args:  ~p~n"
-      "Node 1 NodesPorts: ~p~n"
-      "Node 1 SocketsInfo: ~p~n",
-      [NH1Args, NH1NodesPorts,
-       apply_on_ssl_node(NH1, fun sockets_info/0)]),
-
-    Node1 = NH1#node_handle.nodename,
-    Name1 = lists:takewhile(fun(C) -> C =/= $@ end, atom_to_list(Node1)),
-    {Name1, Port1} = lists:keyfind(Name1, 1, NH1NodesPorts),
-
-    %% Now start a second node, configuring it to use the same port
-    %% number.
-    PortOpt1 = "-kernel inet_dist_listen_min " ++ integer_to_list(Port1) ++
-        " inet_dist_listen_max " ++ integer_to_list(Port1),
-
+    NH2 = start_ssl_node(Config, PortOpts),
     try
-        start_ssl_node(
-          [{tls_verify_opts, PortOpt1}
-          | proplists:delete(tls_verify_opts, Config)])
-    of
-	#node_handle{} = NH2x ->
-	    %% If the node was able to start, it didn't take the port
-	    %% option into account.
-            NH2xArgs = apply_on_ssl_node(NH2x, init, get_arguments, []),
-            {ok, NH2xNodesPorts} = apply_on_ssl_node(NH2x, net_adm, names, []),
-            io:format(
-              "Node 2x Args:  ~p~n"
-              "Node 2x NodesPorts: ~p~n"
-              "Node 2x SocketsInfo: ~p~n",
-              [NH2xArgs, NH2xNodesPorts,
-               apply_on_ssl_node(NH2x, fun sockets_info/0)]),
-	    stop_ssl_node(NH1),
-	    stop_ssl_node(NH2x),
-	    exit(unexpected_success)
-    catch
-	exit:{accept_failed, timeout} ->
-	    %% The node failed to start, as expected.
-	    ok
-    end,
-
-    %% Try again, now specifying a high max port.
-    PortOpt2 = "-kernel inet_dist_listen_min " ++ integer_to_list(Port1) ++
-	" inet_dist_listen_max 65535",
-    NH2 = start_ssl_node([{tls_verify_opts, PortOpt2} |  proplists:delete(tls_verify_opts, Config)]),
-
-    try
-	Node2 = NH2#node_handle.nodename,
-	Name2 = lists:takewhile(fun(C) -> C =/= $@ end, atom_to_list(Node2)),
-	{ok, NodesPorts2} = apply_on_ssl_node(NH2, fun net_adm:names/0),
-	{Name2, Port2} = lists:keyfind(Name2, 1, NodesPorts2),
-
-	%% The new port should be higher:
-	if Port2 > Port1 ->
-		ok;
-	   true ->
-		error({port, Port2, not_higher_than, Port1})
-	end
-    catch
-	_:Reason ->
-	    stop_ssl_node(NH2),
-	    stop_ssl_node(NH1),
-	    ct:fail(Reason)
-    end,
-    stop_ssl_node(NH2),
-    stop_ssl_node(NH1),
-    success(Config).
-
-sockets_info() ->
-    [{element(2, inet:sockname(Socket)),
-      element(2, inet:peername(Socket)),
-      element(2, prim_inet:getstatus(Socket))}
-     ||
-        Socket <-
-            lists:filter(
-              fun (Port) ->
-                      erlang:port_info(Port, name) =:= {name, "tcp_inet"}
-              end, erlang:ports())].
+        basic_test(NH1, NH2, Config),
+        Node2 = NH2#node_handle.nodename,
+        {ok,NodeInfo2} =
+            apply_on_ssl_node(NH1, net_kernel, node_info, [Node2]),
+        {address,#net_address{address = {_,Port}, protocol = tls}} =
+            lists:keyfind(address, 1, NodeInfo2),
+        ok
+    after
+        gen_tcp:close(C),
+        gen_tcp:close(S),
+        stop_ssl_node(NH1),
+        stop_ssl_node(NH2)
+    end.
 
 %%--------------------------------------------------------------------
 listen_options() ->
@@ -380,15 +338,7 @@ connect_options(Config) when is_list(Config) ->
 net_ticker_spawn_options() ->
     [{doc, "Test net_ticker_spawn_options"}].
 net_ticker_spawn_options(Config) when is_list(Config) ->
-    FullsweepString0 = "[{fullsweep_after,0}]",
-    FullsweepString =
-        case os:cmd("echo [{a,1}]") of
-            "[{a,1}]"++_ ->
-                FullsweepString0;
-            _ ->
-                %% Some shells need quoting of [{}]
-                "'"++FullsweepString0++"'"
-        end,
+    FullsweepString = maybe_quote_tuple_list("[{fullsweep_after,0}]"),
     Options = "-kernel net_ticker_spawn_options "++FullsweepString,
     gen_dist_test(net_ticker_spawn_options_test, [{tls_only_basic_opts, Options} | Config]).
 
@@ -681,16 +631,8 @@ plain_verify_options_test(NH1, NH2, _) ->
     [Node1] = apply_on_ssl_node(NH2, fun () -> nodes() end).
 
 do_listen_options(Prio, Config) ->
-    PriorityString0 = "[{priority,"++integer_to_list(Prio)++"}]",
     PriorityString =
-	case os:cmd("echo [{a,1}]") of
-	    "[{a,1}]"++_ ->
-		PriorityString0;
-	    _ ->
-		%% Some shells need quoting of [{}]
-		"'"++PriorityString0++"'"
-	end,
-
+        maybe_quote_tuple_list("[{priority,"++integer_to_list(Prio)++"}]"),
     Options = "-kernel inet_dist_listen_options " ++ PriorityString,
     gen_dist_test(listen_options_test, [{prio, Prio}, {tls_only_basic_opts, Options} | Config]).
 
@@ -712,16 +654,8 @@ listen_options_test(NH1, NH2, Config) ->
     [_|_] = Elevated2.
 
 do_connect_options(Prio, Config) ->
-    PriorityString0 = "[{priority,"++integer_to_list(Prio)++"}]",
     PriorityString =
-	case os:cmd("echo [{a,1}]") of
-	    "[{a,1}]"++_ ->
-		PriorityString0;
-	    _ ->
-		%% Some shells need quoting of [{}]
-		"'"++PriorityString0++"'"
-	end,
-
+        maybe_quote_tuple_list("[{priority,"++integer_to_list(Prio)++"}]"),
     Options = "-kernel inet_dist_connect_options " ++ PriorityString,
     gen_dist_test(connect_options_test,
 		  [{prio, Prio}, {tls_only_basic_opts, Options} | Config]).
@@ -867,11 +801,12 @@ setup_tls_opts(Config) ->
     case proplists:get_value(tls_only_basic_opts, Config, []) of
         [_|_] = BasicOpts -> %% No verify but server still need to have cert
             "-proto_dist inet_tls " ++ "-ssl_dist_opt server_certfile " ++ SC ++ " "
-                ++ "-ssl_dist_opt server_keyfile " ++ SK ++ " " ++ BasicOpts; 
+                ++ "-ssl_dist_opt server_keyfile " ++ SK ++ " " ++ BasicOpts;
         [] -> %% Verify
-             case proplists:get_value(tls_verify_opts, Config, []) of
+            TlsVerifyOpts = proplists:get_value(tls_verify_opts, Config, []),
+             case TlsVerifyOpts of
                  [_|_] ->
-                     BasicVerifyOpts = "-proto_dist inet_tls "
+                     "-proto_dist inet_tls "
                          ++ "-ssl_dist_opt server_certfile " ++ SC ++ " "
                          ++ "-ssl_dist_opt server_keyfile " ++ SK ++ " "
                          ++ "-ssl_dist_opt server_cacertfile " ++ SCA ++ " "
@@ -880,8 +815,8 @@ setup_tls_opts(Config) ->
                          ++ "-ssl_dist_opt client_certfile " ++ CC ++ " "
                          ++ "-ssl_dist_opt client_keyfile " ++ CK ++ " "
                          ++ "-ssl_dist_opt client_cacertfile " ++ CCA ++ " "
-                         ++ "-ssl_dist_opt client_verify verify_peer ",
-                     BasicVerifyOpts ++  proplists:get_value(tls_verify_opts, Config, []);
+                         ++ "-ssl_dist_opt client_verify verify_peer "
+                         ++  TlsVerifyOpts;
                  _ ->  %% No verify, no extra opts
                      "-proto_dist inet_tls " ++ "-ssl_dist_opt server_certfile " ++ SC ++ " "
                          ++ "-ssl_dist_opt server_keyfile " ++ SK ++ " "
@@ -1034,3 +969,11 @@ rsa_intermediate_conf(N) ->
     [{key, ssl_test_lib:hardcode_rsa_key(N)}].
 
 
+maybe_quote_tuple_list(String) ->
+    case os:cmd("echo [{a,1}]") of
+        "[{a,1}]"++_ ->
+            String;
+        _ ->
+            %% Some shells need quoting of [{}]
+            "'"++String++"'"
+    end.
-- 
2.35.3

Places

File 6248-Rewrite-the-listen_port_options-1-test-case.patch of Package erlang (Revision 6cc4e2769dd7ada0d41c527b51da0114)

Places