From 5dd93474eee0b46e7afa62ed9b909f15f9316806 Mon Sep 17 00:00:00 2001 From: Shannon Booth Date: Mon, 17 Jul 2023 16:39:07 +1200 Subject: [PATCH] LibCore+Tests: Unify process handling into Command class The Test262RunnerHandler class in test-test262 was made in order to spawn a subprocess, connect to its input/output error pipes, and obtain its return value. Later on, a copy of this implementation was added to TestSed with modifications, such as adding support for reading from the output pipes as well. Unify these two implementations into a new Core::Command class. This new implementation is more closely modeled from the TestSed implementation due to the extra functionality it implemented. --- Tests/LibJS/test-test262.cpp | 134 +++---------------------- Tests/Utilities/TestSed.cpp | 131 +----------------------- Userland/Libraries/LibCore/Command.cpp | 105 +++++++++++++++++++ Userland/Libraries/LibCore/Command.h | 39 +++++++ 4 files changed, 160 insertions(+), 249 deletions(-) diff --git a/Tests/LibJS/test-test262.cpp b/Tests/LibJS/test-test262.cpp index c6419def1c0..60f91a98947 100644 --- a/Tests/LibJS/test-test262.cpp +++ b/Tests/LibJS/test-test262.cpp @@ -13,15 +13,11 @@ #include #include #include +#include #include -#include -#include #include #include #include -#include -#include -#include enum class TestResult { Passed, @@ -111,117 +107,6 @@ static StringView emoji_for_result(TestResult result) static constexpr StringView total_test_emoji = "🧪"sv; -class Test262RunnerHandler { -public: - static ErrorOr> create(StringView command, char const* const arguments[]) - { - auto write_pipe_fds = TRY(Core::System::pipe2(O_CLOEXEC)); - auto read_pipe_fds = TRY(Core::System::pipe2(O_CLOEXEC)); - - posix_spawn_file_actions_t file_actions; - posix_spawn_file_actions_init(&file_actions); - posix_spawn_file_actions_adddup2(&file_actions, write_pipe_fds[0], STDIN_FILENO); - posix_spawn_file_actions_adddup2(&file_actions, read_pipe_fds[1], STDOUT_FILENO); - - auto pid = TRY(Core::System::posix_spawnp(command, &file_actions, nullptr, const_cast(arguments), environ)); - - posix_spawn_file_actions_destroy(&file_actions); - ArmedScopeGuard runner_kill { [&pid] { kill(pid, SIGKILL); } }; - - TRY(Core::System::close(write_pipe_fds[0])); - TRY(Core::System::close(read_pipe_fds[1])); - - auto infile = TRY(Core::File::adopt_fd(read_pipe_fds[0], Core::File::OpenMode::Read)); - - auto outfile = TRY(Core::File::adopt_fd(write_pipe_fds[1], Core::File::OpenMode::Write)); - - runner_kill.disarm(); - - return make(pid, move(infile), move(outfile)); - } - - Test262RunnerHandler(pid_t pid, NonnullOwnPtr in_file, NonnullOwnPtr out_file) - : m_pid(pid) - , m_input(move(in_file)) - , m_output(move(out_file)) - { - } - - bool write_lines(Span lines) - { - // It's possible the process dies before we can write all the tests - // to the stdin. So make sure that we don't crash but just stop writing. - struct sigaction action_handler { - .sa_handler = SIG_IGN, .sa_mask = {}, .sa_flags = 0, - }; - struct sigaction old_action_handler; - if (sigaction(SIGPIPE, &action_handler, &old_action_handler) < 0) { - perror("sigaction"); - return false; - } - - for (DeprecatedString const& line : lines) { - if (m_output->write_until_depleted(DeprecatedString::formatted("{}\n", line).bytes()).is_error()) - break; - } - - // Ensure that the input stream ends here, whether we were able to write all lines or not - m_output->close(); - - // It's not really a problem if this signal failed - if (sigaction(SIGPIPE, &old_action_handler, nullptr) < 0) - perror("sigaction"); - - return true; - } - - DeprecatedString read_all() - { - auto all_output_or_error = m_input->read_until_eof(); - if (all_output_or_error.is_error()) { - warnln("Got error: {} while reading runner output", all_output_or_error.error()); - return ""sv; - } - return DeprecatedString(all_output_or_error.value().bytes(), Chomp); - } - - enum class ProcessResult { - Running, - DoneWithZeroExitCode, - Failed, - FailedFromTimeout, - Unknown, - }; - - ErrorOr status() - { - if (m_pid == -1) - return ProcessResult::Unknown; - - m_output->close(); - - auto wait_result = TRY(Core::System::waitpid(m_pid, WNOHANG)); - if (wait_result.pid == 0) { - // Attempt to kill it, since it has not finished yet somehow - return ProcessResult::Running; - } - m_pid = -1; - - if (WIFSIGNALED(wait_result.status) && WTERMSIG(wait_result.status) == SIGALRM) - return ProcessResult::FailedFromTimeout; - - if (WIFEXITED(wait_result.status) && WEXITSTATUS(wait_result.status) == 0) - return ProcessResult::DoneWithZeroExitCode; - - return ProcessResult::Failed; - } - -public: - pid_t m_pid; - NonnullOwnPtr m_input; - NonnullOwnPtr m_output; -}; - static ErrorOr> run_test_files(Span files, size_t offset, StringView command, char const* const arguments[]) { HashMap results {}; @@ -234,7 +119,7 @@ static ErrorOr> run_test_files(Span> run_test_files(Spanread_all(); + auto output_or_error = runner_process->read_all(); + DeprecatedString output; + + if (output_or_error.is_error()) + warnln("Got error: {} while reading runner output", output_or_error.error()); + else + output = DeprecatedString(output_or_error.release_value().standard_error.bytes(), Chomp); + auto status_or_error = runner_process->status(); bool failed = false; if (!status_or_error.is_error()) { - VERIFY(status_or_error.value() != Test262RunnerHandler::ProcessResult::Running); - failed = status_or_error.value() != Test262RunnerHandler::ProcessResult::DoneWithZeroExitCode; + VERIFY(status_or_error.value() != Core::Command::ProcessResult::Running); + failed = status_or_error.value() != Core::Command::ProcessResult::DoneWithZeroExitCode; } for (StringView line : output.split_view('\n')) { @@ -285,7 +177,7 @@ static ErrorOr> run_test_files(Span #include +#include #include -#include #include #include -#include -#include - -class Process { -public: - struct ProcessOutputs { - AK::ByteBuffer standard_output; - AK::ByteBuffer standard_error; - }; - - static ErrorOr> create(StringView command, char const* const arguments[]) - { - auto stdin_fds = TRY(Core::System::pipe2(O_CLOEXEC)); - auto stdout_fds = TRY(Core::System::pipe2(O_CLOEXEC)); - auto stderr_fds = TRY(Core::System::pipe2(O_CLOEXEC)); - - posix_spawn_file_actions_t file_actions; - posix_spawn_file_actions_init(&file_actions); - posix_spawn_file_actions_adddup2(&file_actions, stdin_fds[0], STDIN_FILENO); - posix_spawn_file_actions_adddup2(&file_actions, stdout_fds[1], STDOUT_FILENO); - posix_spawn_file_actions_adddup2(&file_actions, stderr_fds[1], STDERR_FILENO); - - auto pid = TRY(Core::System::posix_spawnp(command, &file_actions, nullptr, const_cast(arguments), environ)); - - posix_spawn_file_actions_destroy(&file_actions); - ArmedScopeGuard runner_kill { [&pid] { kill(pid, SIGKILL); } }; - - TRY(Core::System::close(stdin_fds[0])); - TRY(Core::System::close(stdout_fds[1])); - TRY(Core::System::close(stderr_fds[1])); - - auto stdin_file = TRY(Core::File::adopt_fd(stdin_fds[1], Core::File::OpenMode::Write)); - auto stdout_file = TRY(Core::File::adopt_fd(stdout_fds[0], Core::File::OpenMode::Read)); - auto stderr_file = TRY(Core::File::adopt_fd(stderr_fds[0], Core::File::OpenMode::Read)); - - runner_kill.disarm(); - - return make(pid, move(stdin_file), move(stdout_file), move(stderr_file)); - } - - Process(pid_t pid, NonnullOwnPtr stdin_file, NonnullOwnPtr stdout_file, NonnullOwnPtr stderr_file) - : m_pid(pid) - , m_stdin(move(stdin_file)) - , m_stdout(move(stdout_file)) - , m_stderr(move(stderr_file)) - { - } - - ErrorOr write(StringView input) - { - TRY(m_stdin->write_until_depleted(input.bytes())); - m_stdin->close(); - return {}; - } - - bool write_lines(Span lines) - { - // It's possible the process dies before we can write all the tests - // to the stdin. So make sure that we don't crash but just stop writing. - struct sigaction action_handler { - .sa_handler = SIG_IGN, .sa_mask = {}, .sa_flags = 0, - }; - struct sigaction old_action_handler; - if (sigaction(SIGPIPE, &action_handler, &old_action_handler) < 0) { - perror("sigaction"); - return false; - } - - for (DeprecatedString const& line : lines) { - if (m_stdin->write_until_depleted(DeprecatedString::formatted("{}\n", line).bytes()).is_error()) - break; - } - - // Ensure that the input stream ends here, whether we were able to write all lines or not - m_stdin->close(); - - // It's not really a problem if this signal failed - if (sigaction(SIGPIPE, &old_action_handler, nullptr) < 0) - perror("sigaction"); - - return true; - } - - ErrorOr read_all() - { - return ProcessOutputs { TRY(m_stdout->read_until_eof()), TRY(m_stderr->read_until_eof()) }; - } - - enum class ProcessResult { - Running, - DoneWithZeroExitCode, - Failed, - FailedFromTimeout, - Unknown, - }; - - ErrorOr status(int options = 0) - { - if (m_pid == -1) - return ProcessResult::Unknown; - - m_stdin->close(); - - auto wait_result = TRY(Core::System::waitpid(m_pid, options)); - if (wait_result.pid == 0) { - // Attempt to kill it, since it has not finished yet somehow - return ProcessResult::Running; - } - m_pid = -1; - - if (WIFSIGNALED(wait_result.status) && WTERMSIG(wait_result.status) == SIGALRM) - return ProcessResult::FailedFromTimeout; - - if (WIFEXITED(wait_result.status) && WEXITSTATUS(wait_result.status) == 0) - return ProcessResult::DoneWithZeroExitCode; - - return ProcessResult::Failed; - } - -private: - pid_t m_pid; - NonnullOwnPtr m_stdin; - NonnullOwnPtr m_stdout; - NonnullOwnPtr m_stderr; -}; static void run_sed(Vector&& arguments, StringView standard_input, StringView expected_stdout) { MUST(arguments.try_insert(0, "sed")); MUST(arguments.try_append(nullptr)); - auto sed = MUST(Process::create("sed"sv, arguments.data())); + auto sed = MUST(Core::Command::create("sed"sv, arguments.data())); MUST(sed->write(standard_input)); auto [stdout, stderr] = MUST(sed->read_all()); auto status = MUST(sed->status()); - if (status != Process::ProcessResult::DoneWithZeroExitCode) { + if (status != Core::Command::ProcessResult::DoneWithZeroExitCode) { FAIL(DeprecatedString::formatted("sed didn't exit cleanly: status: {}, stdout:{}, stderr: {}", static_cast(status), StringView { stdout.bytes() }, StringView { stderr.bytes() })); } EXPECT_EQ(StringView { expected_stdout.bytes() }, StringView { stdout.bytes() }); diff --git a/Userland/Libraries/LibCore/Command.cpp b/Userland/Libraries/LibCore/Command.cpp index f6ffcce0aa8..9173b5616b4 100644 --- a/Userland/Libraries/LibCore/Command.cpp +++ b/Userland/Libraries/LibCore/Command.cpp @@ -1,5 +1,7 @@ /* * Copyright (c) 2020, Itamar S. + * Copyright (c) 2022, David Tuin + * Copyright (c) 2023, Shannon Booth * * SPDX-License-Identifier: BSD-2-Clause */ @@ -8,6 +10,7 @@ #include #include #include +#include #include #include #include @@ -15,6 +18,108 @@ namespace Core { +ErrorOr> Command::create(StringView command, char const* const arguments[]) +{ + auto stdin_fds = TRY(Core::System::pipe2(O_CLOEXEC)); + auto stdout_fds = TRY(Core::System::pipe2(O_CLOEXEC)); + auto stderr_fds = TRY(Core::System::pipe2(O_CLOEXEC)); + + posix_spawn_file_actions_t file_actions; + posix_spawn_file_actions_init(&file_actions); + posix_spawn_file_actions_adddup2(&file_actions, stdin_fds[0], STDIN_FILENO); + posix_spawn_file_actions_adddup2(&file_actions, stdout_fds[1], STDOUT_FILENO); + posix_spawn_file_actions_adddup2(&file_actions, stderr_fds[1], STDERR_FILENO); + + auto pid = TRY(Core::System::posix_spawnp(command, &file_actions, nullptr, const_cast(arguments), System::environment())); + + posix_spawn_file_actions_destroy(&file_actions); + ArmedScopeGuard runner_kill { [&pid] { kill(pid, SIGKILL); } }; + + TRY(Core::System::close(stdin_fds[0])); + TRY(Core::System::close(stdout_fds[1])); + TRY(Core::System::close(stderr_fds[1])); + + auto stdin_file = TRY(Core::File::adopt_fd(stdin_fds[1], Core::File::OpenMode::Write)); + auto stdout_file = TRY(Core::File::adopt_fd(stdout_fds[0], Core::File::OpenMode::Read)); + auto stderr_file = TRY(Core::File::adopt_fd(stderr_fds[0], Core::File::OpenMode::Read)); + + runner_kill.disarm(); + + return make(pid, move(stdin_file), move(stdout_file), move(stderr_file)); +} + +Command::Command(pid_t pid, NonnullOwnPtr stdin_file, NonnullOwnPtr stdout_file, NonnullOwnPtr stderr_file) + : m_pid(pid) + , m_stdin(move(stdin_file)) + , m_stdout(move(stdout_file)) + , m_stderr(move(stderr_file)) +{ +} + +ErrorOr Command::write(StringView input) +{ + TRY(m_stdin->write_until_depleted(input.bytes())); + m_stdin->close(); + return {}; +} + +bool Command::write_lines(Span lines) +{ + // It's possible the process dies before we can write everything to the + // stdin. So make sure that we don't crash but just stop writing. + + struct sigaction action_handler { }; + action_handler.sa_handler = SIG_IGN; + + struct sigaction old_action_handler; + if (sigaction(SIGPIPE, &action_handler, &old_action_handler) < 0) { + perror("sigaction"); + return false; + } + + for (DeprecatedString const& line : lines) { + if (m_stdin->write_until_depleted(DeprecatedString::formatted("{}\n", line).bytes()).is_error()) + break; + } + + // Ensure that the input stream ends here, whether we were able to write all lines or not + m_stdin->close(); + + // It's not really a problem if this signal failed + if (sigaction(SIGPIPE, &old_action_handler, nullptr) < 0) + perror("sigaction"); + + return true; +} + +ErrorOr Command::read_all() +{ + return ProcessOutputs { TRY(m_stdout->read_until_eof()), TRY(m_stderr->read_until_eof()) }; +} + +ErrorOr Command::status(int options) +{ + if (m_pid == -1) + return ProcessResult::Unknown; + + m_stdin->close(); + + auto wait_result = TRY(Core::System::waitpid(m_pid, options)); + if (wait_result.pid == 0) { + // Attempt to kill it, since it has not finished yet somehow + return ProcessResult::Running; + } + m_pid = -1; + + if (WIFSIGNALED(wait_result.status) && WTERMSIG(wait_result.status) == SIGALRM) + return ProcessResult::FailedFromTimeout; + + if (WIFEXITED(wait_result.status) && WEXITSTATUS(wait_result.status) == 0) + return ProcessResult::DoneWithZeroExitCode; + + return ProcessResult::Failed; +} + // Only supported in serenity mode because we use `posix_spawn_file_actions_addchdir` #ifdef AK_OS_SERENITY diff --git a/Userland/Libraries/LibCore/Command.h b/Userland/Libraries/LibCore/Command.h index 170acc11b06..e670c61efdb 100644 --- a/Userland/Libraries/LibCore/Command.h +++ b/Userland/Libraries/LibCore/Command.h @@ -1,5 +1,7 @@ /* * Copyright (c) 2020, Itamar S. + * Copyright (c) 2022, David Tuin + * Copyright (c) 2023, Shannon Booth * * SPDX-License-Identifier: BSD-2-Clause */ @@ -10,10 +12,13 @@ #include #include #include +#include +#include #include namespace Core { +// FIXME: Unify this and the below 'command' functions with Command class below struct CommandResult { int exit_code { 0 }; ByteBuffer output; @@ -23,4 +28,38 @@ struct CommandResult { ErrorOr command(DeprecatedString const& program, Vector const& arguments, Optional chdir); ErrorOr command(DeprecatedString const& command_string, Optional chdir); +class Command { +public: + struct ProcessOutputs { + ByteBuffer standard_output; + ByteBuffer standard_error; + }; + + static ErrorOr> create(StringView command, char const* const arguments[]); + + Command(pid_t pid, NonnullOwnPtr stdin_file, NonnullOwnPtr stdout_file, NonnullOwnPtr stderr_file); + + ErrorOr write(StringView input); + + bool write_lines(Span lines); + + ErrorOr read_all(); + + enum class ProcessResult { + Running, + DoneWithZeroExitCode, + Failed, + FailedFromTimeout, + Unknown, + }; + + ErrorOr status(int options = 0); + +private: + pid_t m_pid { -1 }; + NonnullOwnPtr m_stdin; + NonnullOwnPtr m_stdout; + NonnullOwnPtr m_stderr; +}; + }