From 66fac5cbab8a9ff4528e70adff8c6cd0e867624f Mon Sep 17 00:00:00 2001 From: Yaroslav Maslennikov Date: Tue, 22 Oct 2024 12:52:48 +0200 Subject: [PATCH 1/3] Add the os_cmd_shell kernel config parameter to point to system shell for os:cmd/2 Initialize the parameter at startup Fix the test - restore the old shell after the test Add initialization to other tests that use os:cmd/2 Update os:cmd/2 description --- lib/kernel/doc/kernel_app.md | 4 ++ lib/kernel/src/kernel.erl | 1 + lib/kernel/src/os.erl | 70 ++++++++++++++-------- lib/kernel/test/os_SUITE.erl | 34 ++++++++++- lib/kernel/test/os_SUITE_data/Makefile.src | 8 ++- lib/kernel/test/os_SUITE_data/sys_shell.c | 6 ++ 6 files changed, 95 insertions(+), 28 deletions(-) create mode 100644 lib/kernel/test/os_SUITE_data/sys_shell.c diff --git a/lib/kernel/doc/kernel_app.md b/lib/kernel/doc/kernel_app.md index 4984a350ab70..0428b911de30 100644 --- a/lib/kernel/doc/kernel_app.md +++ b/lib/kernel/doc/kernel_app.md @@ -512,6 +512,10 @@ For more information about configuration parameters, see file [Escripts and non-interactive I/O in Unicode Usage in Erlang](`e:stdlib:unicode_usage.md#escripts-and-non-interactive-i-o`) for more details. +- **`os_cmd_shell = string()`{: #os_cmd_shell }** - Specifies which shell to + use when invoking system commands via `os:cmd/2`. By default the shell is detected + automatically. + ## Deprecated Configuration Parameters In Erlang/OTP 21.0, a new API for logging was added. The old `error_logger` diff --git a/lib/kernel/src/kernel.erl b/lib/kernel/src/kernel.erl index 021676c33edc..f53f3b10713a 100644 --- a/lib/kernel/src/kernel.erl +++ b/lib/kernel/src/kernel.erl @@ -33,6 +33,7 @@ start(_, []) -> %% Setup the logger and configure the kernel logger environment ok = logger:internal_init_logger(), + ok = os:internal_init_cmd_shell(), case supervisor:start_link({local, kernel_sup}, kernel, []) of {ok, Pid} -> ok = erl_signal_handler:start(), diff --git a/lib/kernel/src/os.erl b/lib/kernel/src/os.erl index adb31bd678f3..3f0895167a3f 100644 --- a/lib/kernel/src/os.erl +++ b/lib/kernel/src/os.erl @@ -37,6 +37,8 @@ a program to run on most platforms. -export([type/0, version/0, cmd/1, cmd/2, find_executable/1, find_executable/2]). +-export([internal_init_cmd_shell/0]). + -include("file.hrl"). -export_type([env_var_name/0, env_var_value/0, env_var_name_value/0]). @@ -518,6 +520,10 @@ cmd(Cmd) -> Executes `Command` in a command shell of the target OS, captures the standard output and standard error of the command, and returns this result as a string. +The command shell can be set using the +[kernel configuration parameter](kernel_app.md#os_cmd_shell), by default the +shell is detected upon system startup. + _Examples:_ ```erlang @@ -581,35 +587,14 @@ get_option(Opt, Options, Default) -> _ -> throw(badopt) end. -mk_cmd({win32,Wtype}, Cmd) -> - Command = case {os:getenv("COMSPEC"),Wtype} of - {false,windows} -> lists:concat(["command.com /c", Cmd]); - {false,_} -> lists:concat(["cmd /c", Cmd]); - {Cspec,_} -> lists:concat([Cspec," /c",Cmd]) - end, +mk_cmd({win32,_}, Cmd) -> + {ok, Shell} = application:get_env(kernel, os_cmd_shell), + Command = lists:concat([Shell, " /c", Cmd]), {Command, [], [], <<>>}; mk_cmd(_,Cmd) -> %% Have to send command in like this in order to make sh commands like %% cd and ulimit available. - %% - %% We use an absolute path here because we do not want the path to be - %% searched in case a stale NFS handle is somewhere in the path before - %% the sh command. - %% - %% Check if the default shell is located in /bin/sh as expected usually - %% or in /system/bin/sh as implemented on Android. The raw option is - %% used to bypass the file server and speed up the file access. - Shell = case file:read_file_info("/bin/sh",[raw]) of - {ok,#file_info{type=regular}} -> - "/bin/sh"; - _ -> - case file:read_file_info("/system/bin/sh",[raw]) of - {ok,#file_info{type=regular}} -> - "/system/bin/sh"; - _ -> - "/bin/sh" - end - end, + {ok, Shell} = application:get_env(kernel, os_cmd_shell), {Shell ++ " -s unix:cmd", [out], %% We insert a new line after the command, in case the command %% contains a comment character. @@ -628,6 +613,41 @@ mk_cmd(_,Cmd) -> ["(", unicode:characters_to_binary(Cmd), "\n) >}. +-doc false. +internal_init_cmd_shell() -> + case application:get_env(kernel, os_cmd_shell) of + undefined -> + application:set_env(kernel, os_cmd_shell, + internal_init_cmd_shell(os:type())); + _ -> + ok + end. +internal_init_cmd_shell({win32,Wtype}) -> + case {os:getenv("COMSPEC"),Wtype} of + {false,windows} -> "command.com"; + {false,_} -> "cmd"; + {Cspec,_} -> Cspec + end; +internal_init_cmd_shell(_) -> + %% We use an absolute path here because we do not want the path to be + %% searched in case a stale NFS handle is somewhere in the path before + %% the sh command. + %% + %% Check if the default shell is located in /bin/sh as expected usually + %% or in /system/bin/sh as implemented on Android. The raw option is + %% used to bypass the file server. + case file:read_file_info("/bin/sh",[raw]) of + {ok,#file_info{type=regular}} -> + "/bin/sh"; + _ -> + case file:read_file_info("/system/bin/sh",[raw]) of + {ok,#file_info{type=regular}} -> + "/system/bin/sh"; + _ -> + "/bin/sh" + end + end. + validate(Term) -> try validate1(Term) catch error:_ -> throw(badarg) diff --git a/lib/kernel/test/os_SUITE.erl b/lib/kernel/test/os_SUITE.erl index 6d6ed337a0cd..8a5691bef78c 100644 --- a/lib/kernel/test/os_SUITE.erl +++ b/lib/kernel/test/os_SUITE.erl @@ -28,7 +28,7 @@ find_executable/1, unix_comment_in_command/1, deep_list_command/1, large_output_command/1, background_command/0, background_command/1, message_leak/1, close_stdin/0, close_stdin/1, max_size_command/1, - perf_counter_api/1, error_info/1]). + perf_counter_api/1, error_info/1, os_cmd_shell/1,os_cmd_shell_peer/1]). -include_lib("common_test/include/ct.hrl"). @@ -43,7 +43,7 @@ all() -> find_executable, unix_comment_in_command, deep_list_command, large_output_command, background_command, message_leak, close_stdin, max_size_command, perf_counter_api, - error_info]. + error_info, os_cmd_shell, os_cmd_shell_peer]. groups() -> []. @@ -469,6 +469,36 @@ error_info(Config) -> ], error_info_lib:test_error_info(os, L). +os_cmd_shell(Config) -> + DataDir = proplists:get_value(data_dir, Config), + SysShell = filename:join(DataDir, "sys_shell"), + + {ok, OldShell} = application:get_env(kernel, os_cmd_shell), + try + application:set_env(kernel, os_cmd_shell, SysShell), + + %% os:cmd should not try to detect the shell location rather than use + %% the value from kernel:os_cmd_shell parameter + comp("sys_shell", os:cmd("ls")) + after + application:set_env(kernel, os_cmd_shell, OldShell) + end. + +os_cmd_shell_peer(Config) -> + DataDir = proplists:get_value(data_dir, Config), + SysShell = "\"" ++ filename:join(DataDir, "sys_shell") ++ "\"", + {ok, Peer, Node} = ?CT_PEER(["-kernel","os_cmd_shell", SysShell]), + try erpc:call(Node, os, cmd, ["ls"], rtnode:timeout(normal)) of + "sys_shell" -> ok; + Other -> ct:fail({unexpected, Other}) + catch + C:R:Stk -> + io:format("~p\n~p\n~p\n", [C,R,Stk]), + ct:fail(failed) + after + peer:stop(Peer) + end. + no_limit_for_opened_files() -> case os:type() of {unix, freebsd} -> diff --git a/lib/kernel/test/os_SUITE_data/Makefile.src b/lib/kernel/test/os_SUITE_data/Makefile.src index f83f781411d9..2141b320bb3d 100644 --- a/lib/kernel/test/os_SUITE_data/Makefile.src +++ b/lib/kernel/test/os_SUITE_data/Makefile.src @@ -3,7 +3,7 @@ LD = @LD@ CFLAGS = @CFLAGS@ -I@erl_include@ @DEFS@ CROSSLDFLAGS = @CROSSLDFLAGS@ -PROGS = my_echo@exe@ my_fds@exe@ +PROGS = my_echo@exe@ my_fds@exe@ sys_shell@exe@ all: $(PROGS) @@ -18,3 +18,9 @@ my_fds@exe@: my_fds@obj@ my_fds@obj@: my_fds.c $(CC) -c -o my_fds@obj@ $(CFLAGS) my_fds.c + +sys_shell@exe@: sys_shell@obj@ + $(LD) $(CROSSLDFLAGS) -o sys_shell sys_shell@obj@ @LIBS@ + +sys_shell@obj@: sys_shell.c + $(CC) -c -o sys_shell@obj@ $(CFLAGS) sys_shell.c diff --git a/lib/kernel/test/os_SUITE_data/sys_shell.c b/lib/kernel/test/os_SUITE_data/sys_shell.c new file mode 100644 index 000000000000..73a8c034501e --- /dev/null +++ b/lib/kernel/test/os_SUITE_data/sys_shell.c @@ -0,0 +1,6 @@ +#include +int main(void) +{ + printf("sys_shell"); + return 0; +} From cf874dca40dc0e6ef9865c1a7c4c9874254b3997 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lukas=20Backstr=C3=B6m?= Date: Mon, 4 Nov 2024 21:30:05 +0100 Subject: [PATCH 2/3] kernel: Re-init shell when config is changed. --- lib/kernel/src/kernel.erl | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/kernel/src/kernel.erl b/lib/kernel/src/kernel.erl index f53f3b10713a..c7ecc855e8c1 100644 --- a/lib/kernel/src/kernel.erl +++ b/lib/kernel/src/kernel.erl @@ -49,6 +49,7 @@ stop(_State) -> %% Some configuration parameters for kernel are changed %%------------------------------------------------------------------- config_change(Changed, New, Removed) -> + ok = os:internal_init_cmd_shell(), do_distribution_change(Changed, New, Removed), do_global_groups_change(Changed, New, Removed), ok. From fc4e1de5b948468793ffcfbf07a700c209acc105 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lukas=20Backstr=C3=B6m?= Date: Mon, 4 Nov 2024 21:31:52 +0100 Subject: [PATCH 3/3] kernel: Use persistent term to store os_cmd_shell value This has two purposes: 1. Make it impossible to change the value through the application:set_env API. 2. Make the lookup slightly faster. --- lib/kernel/src/os.erl | 21 ++++++++++++--------- lib/kernel/test/os_SUITE.erl | 18 ++++++------------ 2 files changed, 18 insertions(+), 21 deletions(-) diff --git a/lib/kernel/src/os.erl b/lib/kernel/src/os.erl index 3f0895167a3f..76dffe851806 100644 --- a/lib/kernel/src/os.erl +++ b/lib/kernel/src/os.erl @@ -587,14 +587,16 @@ get_option(Opt, Options, Default) -> _ -> throw(badopt) end. +-define(KERNEL_OS_CMD_SHELL_KEY, kernel_os_cmd_shell). + mk_cmd({win32,_}, Cmd) -> - {ok, Shell} = application:get_env(kernel, os_cmd_shell), + Shell = persistent_term:get(?KERNEL_OS_CMD_SHELL_KEY), Command = lists:concat([Shell, " /c", Cmd]), {Command, [], [], <<>>}; mk_cmd(_,Cmd) -> %% Have to send command in like this in order to make sh commands like %% cd and ulimit available. - {ok, Shell} = application:get_env(kernel, os_cmd_shell), + Shell = persistent_term:get(?KERNEL_OS_CMD_SHELL_KEY), {Shell ++ " -s unix:cmd", [out], %% We insert a new line after the command, in case the command %% contains a comment character. @@ -615,13 +617,14 @@ mk_cmd(_,Cmd) -> -doc false. internal_init_cmd_shell() -> - case application:get_env(kernel, os_cmd_shell) of - undefined -> - application:set_env(kernel, os_cmd_shell, - internal_init_cmd_shell(os:type())); - _ -> - ok - end. + Shell = + case application:get_env(kernel, os_cmd_shell) of + undefined -> + internal_init_cmd_shell(os:type()); + {ok, Val} -> + Val + end, + persistent_term:put(?KERNEL_OS_CMD_SHELL_KEY, Shell). internal_init_cmd_shell({win32,Wtype}) -> case {os:getenv("COMSPEC"),Wtype} of {false,windows} -> "command.com"; diff --git a/lib/kernel/test/os_SUITE.erl b/lib/kernel/test/os_SUITE.erl index 8a5691bef78c..fe3d81d622ae 100644 --- a/lib/kernel/test/os_SUITE.erl +++ b/lib/kernel/test/os_SUITE.erl @@ -469,21 +469,15 @@ error_info(Config) -> ], error_info_lib:test_error_info(os, L). -os_cmd_shell(Config) -> - DataDir = proplists:get_value(data_dir, Config), - SysShell = filename:join(DataDir, "sys_shell"), +%% Check that is *not* possible to change shell after startup +os_cmd_shell(_Config) -> - {ok, OldShell} = application:get_env(kernel, os_cmd_shell), - try - application:set_env(kernel, os_cmd_shell, SysShell), + application:set_env(kernel, os_cmd_shell, "broken shell"), - %% os:cmd should not try to detect the shell location rather than use - %% the value from kernel:os_cmd_shell parameter - comp("sys_shell", os:cmd("ls")) - after - application:set_env(kernel, os_cmd_shell, OldShell) - end. + %% os:cmd should continue to work as normal + comp("hello", os:cmd("echo hello")). +%% When started with os_cmd_shell set, we make sure that it is used. os_cmd_shell_peer(Config) -> DataDir = proplists:get_value(data_dir, Config), SysShell = "\"" ++ filename:join(DataDir, "sys_shell") ++ "\"",