kernel: Improve error handling for -remsh

5 files changed, 187 insertions, 65 deletions

diff --git a/lib/kernel/src/user_drv.erl b/lib/kernel/src/user_drv.erl
index 79bfb68d33..5e8e01a7d7 100644
--- a/lib/kernel/src/user_drv.erl
+++ b/lib/kernel/src/user_drv.erl
@@ -99,6 +99,9 @@ start() ->
     case init:get_argument(remsh) of
         {ok,[[Node]]} ->
             start(#{ initial_shell => {remote, Node} });
+        {ok,[[Node]|_]} ->
+            ?LOG_WARNING("Multiple -remsh given to erl, using the first, ~p", [Node]),
+            start(#{ initial_shell => {remote, Node} });
         E when E =:= error ; E =:= {ok,[[]]} ->
             start(#{ })
     end.
@@ -149,6 +152,7 @@ init(Args) ->
     %% When running in embedded mode we need to call prim_tty:on_load manually here
     %% as the automatic call happens after user is started.
     ok = init:run_on_load_handlers([prim_tty]),
+
     IsTTY = prim_tty:isatty(stdin) =:= true andalso prim_tty:isatty(stdout) =:= true,
     StartShell = maps:get(initial_shell, Args, undefined) =/= noshell,
     OldShell = maps:get(initial_shell, Args, undefined) =:= oldshell,
@@ -208,7 +212,12 @@ init(internal, TTYState, {Args, State = #state{ user = User }}) ->
         #{ initial_shell := noshell } ->
             init_noshell(NewState);
         #{ initial_shell := {remote, Node} } ->
-            init_remote_shell(NewState, Node);
+            InitialShell = {shell,start,[]},
+            exit_on_remote_shell_error(
+              Node, InitialShell, init_remote_shell(NewState, Node, InitialShell));
+        #{ initial_shell := {remote, Node, InitialShell} } ->
+            exit_on_remote_shell_error(
+              Node, InitialShell, init_remote_shell(NewState, Node, InitialShell));
         #{ initial_shell := oldshell } ->
             old = State#state.shell_started,
             init_local_shell(NewState, {shell,start,[]});
@@ -218,21 +227,29 @@ init(internal, TTYState, {Args, State = #state{ user = User }}) ->
             init_local_shell(NewState, {shell,start,[init]})
     end.
 
+exit_on_remote_shell_error(RemoteNode, _, {error, noconnection}) ->
+    io:format(standard_error, "Could not connect to ~p\n", [RemoteNode]),
+    erlang:halt(1);
+exit_on_remote_shell_error(RemoteNode, {M, _, _}, {error, Reason}) ->
+    io:format(standard_error, "Could not load ~p on ~p (~p)\n", [RemoteNode, M, Reason]),
+    erlang:halt(1);
+exit_on_remote_shell_error(_, _, Result) ->
+    Result.
+
 %% We have been started with -noshell. In this mode the current_group is
 %% the `user` group process.
 init_noshell(State) ->
     init_shell(State#state{ shell_started = false }, "").
 
-init_remote_shell(State, Node) ->
+init_remote_shell(State, Node, {M, F, A}) ->
 
-    StartedDist =
-        case net_kernel:get_state() of
-            #{ started := no } ->
-                {ok, _} = net_kernel:start([undefined, shortnames]),
-                true;
-            _ ->
-                false
-        end,
+    case net_kernel:get_state() of
+        #{ started := no } ->
+            {ok, _} = net_kernel:start([undefined, shortnames]),
+            ok;
+        _ ->
+            ok
+    end,
 
     LocalNode =
         case net_kernel:get_state() of
@@ -253,27 +270,44 @@ init_remote_shell(State, Node) ->
     case net_kernel:connect_node(RemoteNode) of
         true ->
 
-            %% We fetch the shell slogan from the remote node
-            Slogan =
-                case erpc:call(RemoteNode, application, get_env,
-                               [stdlib, shell_slogan,
-                                erpc:call(RemoteNode, erlang, system_info, [system_version])]) of
-                    Fun when is_function(Fun, 0) ->
-                        erpc:call(RemoteNode, Fun);
-                    SloganEnv ->
-                        SloganEnv
-                end,
-
-            RShell = {RemoteNode,shell,start,[]},
-            Gr = gr_add_cur(State#state.groups,
-                            group:start(self(), RShell, [{echo,State#state.shell_started =:= new}] ++ remsh_opts(RemoteNode)),
-                            RShell),
-
-            init_shell(State#state{ groups = Gr }, [Slogan,$\n]);
+            case erpc:call(RemoteNode, code, ensure_loaded, [M]) of
+                {error, Reason} when Reason =/= embedded ->
+                    {error, Reason};
+                _ ->
+
+                    %% Setup correct net tick times
+                    case erpc:call(RemoteNode, net_kernel, get_net_ticktime, []) of
+                        {ongoing_change_to, NetTickTime} ->
+                            _ = net_kernel:set_net_ticktime(NetTickTime),
+                            ok;
+                        NetTickTime ->
+                            _ = net_kernel:set_net_ticktime(NetTickTime),
+                            ok
+                    end,
+
+                    RShell = {RemoteNode, M, F, A},
+
+                    %% We fetch the shell slogan from the remote node
+                    Slogan =
+                        case erpc:call(RemoteNode, application, get_env,
+                                       [stdlib, shell_slogan,
+                                        erpc:call(RemoteNode, erlang, system_info, [system_version])]) of
+                            Fun when is_function(Fun, 0) ->
+                                erpc:call(RemoteNode, Fun);
+                            SloganEnv ->
+                                SloganEnv
+                        end,
+
+                    Group = group:start(self(), RShell,
+                                        [{echo,State#state.shell_started =:= new}] ++
+                                            remsh_opts(RemoteNode)),
+
+                    Gr = gr_add_cur(State#state.groups, Group, RShell),
+
+                    init_shell(State#state{ groups = Gr }, [Slogan,$\n])
+            end;
         false ->
-            ?LOG_ERROR("Could not connect to ~p, starting local shell",[RemoteNode]),
-            _ = [net_kernel:stop() || StartedDist],
-            init_local_shell(State, {shell, start, []})
+            {error, noconnection}
     end.
 
 init_local_shell(State, InitialShell) ->
@@ -346,27 +380,40 @@ server({call, From}, {start_shell, Args},
                        read = ReadHandle,
                        write = WriteHandle
                       },
-    R = case maps:get(initial_shell, Args, undefined) of
-            noshell ->
-                init_noshell(NewHandleState);
-            {remote, Node} ->
-                init_remote_shell(NewHandleState, Node);
-            undefined ->
-                case NewHandleState#state.shell_started of
-                    old ->
-                        init_local_shell(NewHandleState, {shell,start,[]});
-                    new ->
-                        init_local_shell(NewHandleState, {shell,start,[init]});
-                    false ->
-                        %% This can never happen, but dialyzer complains so we add
-                        %% this clause.
-                        keep_state_and_data
-                end;
-            InitialShell ->
-                init_local_shell(NewHandleState, InitialShell)
+    {Result, Reply}
+        = case maps:get(initial_shell, Args, undefined) of
+              noshell ->
+                  {init_noshell(NewHandleState), ok};
+              {remote, Node} ->
+                  case init_remote_shell(NewHandleState, Node, {shell, start, []}) of
+                      {error, _} = Error ->
+                          {init_noshell(NewHandleState), Error};
+                      R ->
+                          {R, ok}
+                  end;
+              {remote, Node, InitialShell} ->
+                  case init_remote_shell(NewHandleState, Node, InitialShell) of
+                      {error, _} = Error ->
+                          {init_noshell(NewHandleState), Error};
+                      R ->
+                          {R, ok}
+                  end;
+              undefined ->
+                  case NewHandleState#state.shell_started of
+                      old ->
+                          {init_local_shell(NewHandleState, {shell,start,[]}), ok};
+                      new ->
+                          {init_local_shell(NewHandleState, {shell,start,[init]}), ok};
+                      false ->
+                          %% This can never happen, but dialyzer complains so we add
+                          %% this clause.
+                          {keep_state_and_data, ok}
+                  end;
+              InitialShell ->
+                  {init_local_shell(NewHandleState, InitialShell), ok}
         end,
-    gen_statem:reply(From, ok),
-    R;
+    gen_statem:reply(From, Reply),
+    Result;
 server({call, From}, {start_shell, _Args}, _State) ->
     gen_statem:reply(From, {error, already_started}),
     keep_state_and_data;
diff --git a/lib/kernel/test/interactive_shell_SUITE.erl b/lib/kernel/test/interactive_shell_SUITE.erl
index 5d64eb903f..6bced19baf 100644
--- a/lib/kernel/test/interactive_shell_SUITE.erl
+++ b/lib/kernel/test/interactive_shell_SUITE.erl
@@ -55,7 +55,7 @@
          shell_delete_unicode_not_at_cursor_wrap/1,
          shell_update_window_unicode_wrap/1,
          shell_standard_error_nlcr/1,
-         remsh_basic/1, remsh_longnames/1, remsh_no_epmd/1,
+         remsh_basic/1, remsh_error/1, remsh_longnames/1, remsh_no_epmd/1,
          remsh_expand_compatibility_25/1, remsh_expand_compatibility_later_version/1,
          external_editor/1, external_editor_visual/1,
          external_editor_unicode/1, shell_ignore_pager_commands/1]).
@@ -96,6 +96,7 @@ groups() ->
        shell_history_custom_errors]},
      {remsh, [],
       [remsh_basic,
+       remsh_error,
        remsh_longnames,
        remsh_no_epmd,
        remsh_expand_compatibility_25,
@@ -2073,10 +2074,20 @@ remsh_basic(Config) when is_list(Config) ->
     %% Test that remsh works without -sname.
     rtnode:run(PreCmds ++ PostCmds, [], " ", "-remsh " ++ TargetNodeStr),
 
+    %% Test that if multiple remsh are given, we select the first
+    rtnode:run([{expect, "Multiple"}] ++ PreCmds ++ PostCmds,
+               [], " ",
+               "-remsh " ++ TargetNodeStr ++ " -remsh invalid_node"),
+
     peer:stop(Peer),
 
     ok.
 
+%% Test that if we cannot connect to a node, we get a correct error
+remsh_error(_Config) ->
+    "Could not connect to \"invalid_node\"\n" =
+        os:cmd(ct:get_progname() ++ " -remsh invalid_node").
+
 quit_hosting_node() ->
     %% Command sequence for entering a shell on the hosting node.
     [{putdata, "\^g"},
diff --git a/lib/stdlib/doc/src/shell.xml b/lib/stdlib/doc/src/shell.xml
index 900483c58e..3c719fa63f 100644
--- a/lib/stdlib/doc/src/shell.xml
+++ b/lib/stdlib/doc/src/shell.xml
@@ -967,7 +967,9 @@ q                 - quit erlang
     </func>
 
     <func>
-      <name name="start_interactive" arity="1" since="OTP @OTP-17932@"/>
+      <name name="start_interactive" arity="1" clause_i="1" since="OTP @OTP-17932@"/>
+      <name name="start_interactive" arity="1" clause_i="2" since="OTP @OTP-17932@"/>
+      <name name="start_interactive" arity="1" clause_i="3" since="OTP @OTP-17932@"/>
       <fsummary>Start the interactive shell</fsummary>
       <desc>
         <p>Starts the interactive shell if it has not already been started.
@@ -1006,6 +1008,29 @@ q                 - quit erlang
                 <seecom marker="erts:erl#remsh"><c>-remsh</c></seecom>
                 was given to <seecom marker="erts:erl"><c>erl</c></seecom>.</p>
           </item>
+          <tag>{remote,
+                <seetype marker="erts:erlang#string"><c>string()</c></seetype>,
+                <seetype marker="erts:erlang#mfa"><c>mfa()</c></seetype>}</tag>
+          <item>
+              <p>Starts the interactive shell using as if
+                <seecom marker="erts:erl#remsh"><c>-remsh</c></seecom>
+                was given to <seecom marker="erts:erl"><c>erl</c></seecom>
+                but with an alternative shell implementation.</p>
+          </item>
+        </taglist>
+        <p>On error this function will return:</p>
+        <taglist>
+            <tag>already_started</tag>
+            <item>if an interactive shell is already started.</item>
+            <tag>noconnection</tag>
+            <item>if a remote shell was requested but it could not be connected to.</item>
+            <tag>badfile | nofile | on_load_failure</tag>
+            <item>if a remote shell was requested with a custom
+              <seetype marker="erts:erlang#mfa">mfa()</seetype>,
+              but the module could not be loaded. See <seeerl marker="kernel:code#error_reasons">
+              Error Reasons for Code-Loading Functions</seeerl>
+              for a description of the error reasons.
+            </item>
         </taglist>
       </desc>
     </func>
diff --git a/lib/stdlib/src/shell.erl b/lib/stdlib/src/shell.erl
index 34db07883c..5d5874a086 100644
--- a/lib/stdlib/src/shell.erl
+++ b/lib/stdlib/src/shell.erl
@@ -54,12 +54,15 @@ non_local_allowed({init,stop},[],State) ->
 non_local_allowed(_,_,State) ->
     {false,State}.
 
--spec start_interactive() -> ok | {error, already_started | enottty}.
+-spec start_interactive() -> ok | {error, already_started}.
 start_interactive() ->
     user_drv:start_shell().
--spec start_interactive(noshell | mfa() | {node(), mfa()} |
-                        {remote, string()} | {remote, string(), mfa()}) ->
-          ok | {error, already_started}.
+-spec start_interactive(noshell | mfa()) ->
+          ok | {error, already_started};
+                       ({remote, string()}) ->
+          ok | {error, already_started | noconnection};
+                       ({node(), mfa()} | {remote, string(), mfa()}) ->
+          ok | {error, already_started | noconnection | badfile | nofile | on_load_failure}.
 start_interactive({Node, {M, F, A}}) ->
     user_drv:start_shell(#{ initial_shell => {Node, M, F ,A} });
 start_interactive(InitialShell) ->
diff --git a/lib/stdlib/test/shell_SUITE.erl b/lib/stdlib/test/shell_SUITE.erl
index a90a89fa06..0b1a08d6e7 100644
--- a/lib/stdlib/test/shell_SUITE.erl
+++ b/lib/stdlib/test/shell_SUITE.erl
@@ -3080,25 +3080,29 @@ start_interactive(_Config) ->
     start_interactive_shell(["-env","TERM","dumb"]).
 
 start_interactive_shell(ExtraArgs) ->
+
+    %% Basic test case
     rtnode:run(
-      [{expect, "test"},
+      [{expect, "eval_test"},
        {putline, "test."},
        {eval, fun() -> shell:start_interactive() end},
        {expect, "1>"},
        {expect, "2>"},
        {eval, fun() -> {error,already_started} = shell:start_interactive(), ok end}
-      ],[],"",["-noinput","-eval","io:format(\"test~n\")"] ++ ExtraArgs),
+      ],[],"",["-noinput","-eval","io:format(\"eval_test~n\")"] ++ ExtraArgs),
 
+    %% Test that custom MFA works
     rtnode:run(
-      [{expect, "test"},
+      [{expect, "eval_test"},
        {putline, "test."},
        {eval, fun() -> shell:start_interactive({shell,start,[]}) end},
        {expect, "1>"},
        {expect, "2>"}
-      ],[],"",["-noinput","-eval","io:format(\"test~n\")"] ++ ExtraArgs),
+      ],[],"",["-noinput","-eval","io:format(\"eval_test~n\")"] ++ ExtraArgs),
 
+    %% Test that we can start noshell and then a shell
     rtnode:run(
-      [{expect, "test"},
+      [{expect, "eval_test"},
        {putline, "test."},
        {eval, fun() -> shell:start_interactive(noshell) end},
        {eval, fun() -> io:format(user,"~ts",[io:get_line(user, "")]) end},
@@ -3107,25 +3111,57 @@ start_interactive_shell(ExtraArgs) ->
        {eval, fun() -> shell:start_interactive() end},
        {expect, "1>"},
        {expect, "2>"}
-      ],[],"",["-noinput","-eval","io:format(\"test~n\")"] ++ ExtraArgs),
+      ],[],"",["-noinput","-eval","io:format(\"eval_test~n\")"] ++ ExtraArgs),
 
+    %% Test that we can start various remote shell combos
     [ begin
           {ok, Peer, Node} = ?CT_PEER(),
-          unlink(Peer),
           SNode = atom_to_list(Node),
           rtnode:run(
-            [{expect, "test"},
+            [{expect, "eval_test"},
              {putline, "test."},
              {eval, fun() -> shell:start_interactive(Arg(Node)) end},
              {expect, "\\Q("++SNode++")\\E2>"}
-            ],[],"",["-noinput","-eval","io:format(\"test~n\")"] ++ ExtraArgs)
+            ] ++ quit_hosting_node(),
+            [],"",["-noinput","-eval","io:format(\"eval_test~n\")"] ++ ExtraArgs),
+          peer:stop(Peer)
       end || Arg <- [fun(Node) -> {Node, {shell,start,[]}} end,
                      fun(Node) -> {remote, atom_to_list(Node)} end,
                      fun(Node) -> {remote, hd(string:split(atom_to_list(Node),"@"))} end,
                      fun(Node) -> {remote, atom_to_list(Node), {shell,start,[]}} end
                     ]],
+
+    %% Test that errors work as they should
+    {ok, Peer, Node} = ?CT_PEER(),
+    rtnode:run(
+      [{expect, "eval_test"},
+       {eval, fun() ->
+                      {error,noconnection} = shell:start_interactive(
+                                               {remote,"invalid_node"}),
+                      {error,noconnection} = shell:start_interactive(
+                                               {remote,"invalid_node",
+                                                {invalid_module, start, []}}),
+                      {error,nofile} = shell:start_interactive(
+                                         {remote,atom_to_list(Node),
+                                          {invalid_module, start, []}}),
+                      shell:start_interactive({remote, atom_to_list(Node)})
+              end},
+       {expect, "1> $"}
+      ] ++ quit_hosting_node(),
+      [],"",["-noinput","-eval","io:format(\"eval_test~n\")"] ++ ExtraArgs),
+    peer:stop(Peer),
+
     ok.
 
+quit_hosting_node() ->
+    [{putline, "\^g"},
+     {expect, "--> $"},
+     {putline, "s"},
+     {expect, "--> $"},
+     {putline, "c"},
+     {expect, ["Eshell"]},
+     {expect, ["1> $"]}].
+
 term_to_string(T) ->
     lists:flatten(io_lib:format("~w", [T])).