diff --git a/include/linuxdeploy/subprocess/process.h b/include/linuxdeploy/subprocess/process.h index 8e773fa..e1d400b 100644 --- a/include/linuxdeploy/subprocess/process.h +++ b/include/linuxdeploy/subprocess/process.h @@ -25,14 +25,6 @@ namespace linuxdeploy { // these constants help make the pipe code more readable static constexpr int READ_END_ = 0, WRITE_END_ = 1; - static std::vector make_args_vector_(const std::vector& args); - - static std::vector make_env_vector_(const subprocess_env_map_t& env); - - static int check_waitpid_status_(int status); - - static void close_pipe_fd_(int fd); - public: /** * Create a child process. diff --git a/src/plugin/plugin_process_handler.cpp b/src/plugin/plugin_process_handler.cpp index f0d5dbc..2b15785 100644 --- a/src/plugin/plugin_process_handler.cpp +++ b/src/plugin/plugin_process_handler.cpp @@ -58,15 +58,15 @@ namespace linuxdeploy { for (;;) { for (auto& pipe_to_be_logged : pipes_to_be_logged) { + if (pipe_to_be_logged.eof) { + continue; + } + const auto log_prefix = "[" + name_ + "/" + pipe_to_be_logged.stream_name_ + "] "; // since we have our own ldLog instance for every pipe, we can get away with this rather small read buffer subprocess::subprocess_result_buffer_t intermediate_buffer(4096); - if (pipe_to_be_logged.eof) { - break; - } - // (try to) read from pipe const auto bytes_read = pipe_to_be_logged.reader_.read(intermediate_buffer); diff --git a/src/subprocess/process.cpp b/src/subprocess/process.cpp index 1502ec5..137d14f 100644 --- a/src/subprocess/process.cpp +++ b/src/subprocess/process.cpp @@ -17,6 +17,91 @@ // shorter than using namespace ... using namespace linuxdeploy::subprocess; +namespace { + std::vector make_args_vector(const std::vector& args) { + std::vector rv{}; + rv.reserve(args.size()); + + for (const auto& arg : args) { + rv.emplace_back(strdup(arg.c_str())); + } + + // execv* want a nullptr-terminated array + rv.emplace_back(nullptr); + + return rv; + } + + std::vector make_env_vector(const subprocess_env_map_t& env) { + std::vector rv; + + // first, copy existing environment + // we cannot reserve space in the vector unfortunately, as we don't know the size of environ before the iteration + if (environ != nullptr) { + for (auto** current_env_var = environ; *current_env_var != nullptr; ++current_env_var) { + rv.emplace_back(strdup(*current_env_var)); + } + } + + // add own environment variables, overwriting existing ones if necessary + for (const auto& env_var : env) { + const auto& key = env_var.first; + const auto& value = env_var.second; + + auto predicate = [&key](char* existing_env_var) { + char* equal_sign = strstr(existing_env_var, "="); + + if (equal_sign == nullptr) { + throw std::runtime_error{"no equal sign in environment variable"}; + } + + return strncmp(existing_env_var, key.c_str(), std::distance(equal_sign, existing_env_var)) == 0; + }; + + // delete existing env var, if any + rv.erase(std::remove_if(rv.begin(), rv.end(), predicate), rv.end()); + + // insert new value + std::ostringstream oss; + oss << key; + oss << "="; + oss << value; + + rv.emplace_back(strdup(oss.str().c_str())); + } + + // exec*e want a nullptr-terminated array + rv.emplace_back(nullptr); + + return rv; + } + + int check_waitpid_status(int status) { + if (WIFSIGNALED(status) != 0) { + // TODO: consider treating child exit caused by signals separately + return WTERMSIG(status); + } else if (WIFEXITED(status) != 0) { + return WEXITSTATUS(status); + } + + throw std::logic_error{"unknown child process state"}; + } + + void close_pipe_fd(int fd, bool ignore_ebadf = false) { + const auto rv = ::close(fd); + + if (rv != 0) { + const auto error = errno; + + // might be closed already by the child process, in this case we can safely ignore the error + if (error != EBADF) { + throw std::logic_error("failed to close pipe fd: " + std::string(strerror(error))); + }; + } + } + +} + int process::pid() const { return child_pid_; } @@ -66,8 +151,8 @@ process::process(const std::vector& args, const subprocess_env_map_ // we're in the child process // first step: close the read end of both pipes - close_pipe_fd_(stdout_pipe_fds[READ_END_]); - close_pipe_fd_(stderr_pipe_fds[READ_END_]); + close_pipe_fd(stdout_pipe_fds[READ_END_]); + close_pipe_fd(stderr_pipe_fds[READ_END_]); auto connect_fd = [](int fd, int fileno) { for (;;) { @@ -87,12 +172,12 @@ process::process(const std::vector& args, const subprocess_env_map_ connect_fd(stderr_pipe_fds[WRITE_END_], STDERR_FILENO); // now, we also have to close the write end of both pipes - close_pipe_fd_(stdout_pipe_fds[WRITE_END_]); - close_pipe_fd_(stderr_pipe_fds[WRITE_END_]); + close_pipe_fd(stdout_pipe_fds[WRITE_END_]); + close_pipe_fd(stderr_pipe_fds[WRITE_END_]); // prepare arguments for exec* - auto exec_args = make_args_vector_(args); - auto exec_env = make_env_vector_(env); + auto exec_args = make_args_vector(args); + auto exec_env = make_env_vector(env); // call subprocess execvpe(args.front().c_str(), exec_args.data(), exec_env.data()); @@ -114,9 +199,9 @@ process::process(const std::vector& args, const subprocess_env_map_ // parent code - // we do not intend to write to the processes - close_pipe_fd_(stdout_pipe_fds[WRITE_END_]); - close_pipe_fd_(stderr_pipe_fds[WRITE_END_]); + // we do not intend to write to these pipes from this end + close_pipe_fd(stdout_pipe_fds[WRITE_END_]); + close_pipe_fd(stderr_pipe_fds[WRITE_END_]); // store file descriptors stdout_fd_ = stdout_pipe_fds[READ_END_]; @@ -125,24 +210,22 @@ process::process(const std::vector& args, const subprocess_env_map_ int process::close() { if (!exited_) { - close_pipe_fd_(stdout_fd_); - stdout_fd_ = -1; + int status; - close_pipe_fd_(stderr_fd_); - stderr_fd_ = -1; - - { - int status; - - if (waitpid(child_pid_, &status, 0) == -1) { - throw std::logic_error{"waitpid() failed"}; - } - - exited_ = true; - exit_code_ = check_waitpid_status_(status); + if (waitpid(child_pid_, &status, 0) == -1) { + throw std::logic_error{"waitpid() failed"}; } + + exited_ = true; + exit_code_ = check_waitpid_status(status); } + close_pipe_fd(stdout_fd_, true); + stdout_fd_ = -1; + + close_pipe_fd(stderr_fd_, true); + stderr_fd_ = -1; + return exit_code_; } @@ -150,63 +233,6 @@ process::~process() { (void) close(); } -std::vector process::make_args_vector_(const std::vector& args) { - std::vector rv{}; - rv.reserve(args.size()); - - for (const auto& arg : args) { - rv.emplace_back(strdup(arg.c_str())); - } - - // execv* want a nullptr-terminated array - rv.emplace_back(nullptr); - - return rv; -} - -std::vector process::make_env_vector_(const subprocess_env_map_t& env) { - std::vector rv; - - // first, copy existing environment - // we cannot reserve space in the vector unfortunately, as we don't know the size of environ before the iteration - if (environ != nullptr) { - for (auto** current_env_var = environ; *current_env_var != nullptr; ++current_env_var) { - rv.emplace_back(strdup(*current_env_var)); - } - } - - // add own environment variables, overwriting existing ones if necessary - for (const auto& env_var : env) { - const auto& key = env_var.first; - const auto& value = env_var.second; - - auto predicate = [&key](char* existing_env_var) { - char* equal_sign = strstr(existing_env_var, "="); - - if (equal_sign == nullptr) { - throw std::runtime_error{"no equal sign in environment variable"}; - } - - return strncmp(existing_env_var, key.c_str(), std::distance(equal_sign, existing_env_var)) == 0; - }; - - // delete existing env var, if any - rv.erase(std::remove_if(rv.begin(), rv.end(), predicate), rv.end()); - - // insert new value - std::ostringstream oss; - oss << key; - oss << "="; - oss << value; - - rv.emplace_back(strdup(oss.str().c_str())); - } - - // exec*e want a nullptr-terminated array - rv.emplace_back(nullptr); - - return rv; -} void process::kill(int signal) const { if (::kill(child_pid_, signal) != 0) { @@ -231,15 +257,8 @@ bool process::is_running() { } if (result == child_pid_) { - // TODO: extract the following lines from both this method and close() to eliminate duplicate code - close_pipe_fd_(stdout_fd_); - stdout_fd_ = -1; - - close_pipe_fd_(stderr_fd_); - stderr_fd_ = -1; - exited_ = true; - exit_code_ = check_waitpid_status_(status); + exit_code_ = check_waitpid_status(status); return false; } @@ -252,23 +271,3 @@ bool process::is_running() { // can only happen if waitpid() returns an unknown process ID throw std::logic_error{"unknown error occured"}; } - -int process::check_waitpid_status_(int status) { - if (WIFSIGNALED(status) != 0) { - // TODO: consider treating child exit caused by signals separately - return WTERMSIG(status); - } else if (WIFEXITED(status) != 0) { - return WEXITSTATUS(status); - } - - throw std::logic_error{"unknown child process state"}; -} - -void process::close_pipe_fd_(int fd) { - const auto rv = ::close(fd); - - if (rv != 0) { - const auto error = errno; - throw std::logic_error("failed to close pipe fd: " + std::string(strerror(error))); - } -}