From 4afebf1779a227c0bbdbd0acf61f1486d45187d4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marius=20Miku=C4=8Dionis?= Date: Wed, 23 Oct 2024 16:49:48 +0200 Subject: [PATCH] refactor: popen and pclose error reporting * Set errno for popen and pclose for WIN32 just like POSIX does (at least mingw64+wine seems to decode the error message without doing anything extra) * A work around for popen/pclose: complicated but works * Refactored popen/pclose into custom implementation and added c++ wrappers * Removed exceptions and cleaned up process pipe api * Refactored popen/pclose into C++ class * Fixed trivial issues for Linux * Simplified the move semantics by avoiding virtual functions * Moved error reporting into common_pipe * Simplified open and close in case the pipe is already opened/closed * Fixed error handling for MSVC, which very weird: MSVC rejects strerror, suggest to use "secure" strerror_s, but then does not supply strerrorlen_s GCC does not provide strerror_s, looks like strerror is good enough there. * MSVC is not following any standards --- source/matplot/backend/gnuplot.cpp | 47 ++--- source/matplot/backend/gnuplot.h | 5 +- source/matplot/util/common.cpp | 22 +- source/matplot/util/popen.cpp | 328 +++++++++++++++++++++-------- source/matplot/util/popen.h | 96 +++++++-- 5 files changed, 354 insertions(+), 144 deletions(-) diff --git a/source/matplot/backend/gnuplot.cpp b/source/matplot/backend/gnuplot.cpp index 1e4a3300..674e1ff2 100644 --- a/source/matplot/backend/gnuplot.cpp +++ b/source/matplot/backend/gnuplot.cpp @@ -4,12 +4,13 @@ #include "gnuplot.h" -#include -#include #include #include +#include #include #include +#include +#include #ifdef __has_include #if __has_include() @@ -70,17 +71,18 @@ namespace matplot::backend { } // Open the gnuplot pipe_ + int perr; if constexpr (windows_should_persist_by_default) { - pipe_ = POPEN("gnuplot --persist", "w"); + perr = pipe_.open("gnuplot --persist"); } else { - pipe_ = POPEN("gnuplot", "w"); + perr = pipe_.open("gnuplot"); } // Check if everything is OK - if (!pipe_) { - std::cerr << "Opening the gnuplot pipe_ failed!" << std::endl; - std::cerr - << "Please install gnuplot 5.2.6+: http://www.gnuplot.info" + if (perr != 0 || !pipe_.opened()) { + std::cerr << "Opening the gnuplot failed: "; + std::cerr << pipe_.error() << std::endl; + std::cerr << "Please install gnuplot 5.2.6+: http://www.gnuplot.info" << std::endl; } } @@ -97,9 +99,6 @@ namespace matplot::backend { flush_commands(); run_command("exit"); flush_commands(); - if (pipe_) { - PCLOSE(pipe_); - } } bool gnuplot::is_interactive() { return output_.empty(); } @@ -281,8 +280,7 @@ namespace matplot::backend { if constexpr (dont_let_it_close_too_fast) { last_flush_ = std::chrono::high_resolution_clock::now(); } - fputs("\n", pipe_); - fflush(pipe_); + pipe_.flush("\n"); if constexpr (trace_commands) { std::cout << "\n\n\n\n" << std::endl; } @@ -294,19 +292,19 @@ namespace matplot::backend { } void gnuplot::run_command(const std::string &command) { - if (!pipe_) { + if (!pipe_.opened()) { return; } - size_t pipe_capacity = gnuplot_pipe_capacity(pipe_); + size_t pipe_capacity = gnuplot_pipe_capacity(pipe_.file()); if (command.size() + bytes_in_pipe_ > pipe_capacity) { flush_commands(); bytes_in_pipe_ = 0; } if (!command.empty()) { - fputs(command.c_str(), pipe_); + pipe_.write(command); } - // fputs("; ", pipe_); - fputs("\n", pipe_); + // proc_write(&pipe_, "; "); + pipe_.write("\n"); bytes_in_pipe_ += command.size(); if constexpr (trace_commands) { std::cout << command << std::endl; @@ -323,11 +321,9 @@ namespace matplot::backend { static std::string terminal_type; const bool dont_know_term_type = terminal_type.empty(); if (dont_know_term_type) { - terminal_type = - run_and_get_output("gnuplot -e \"show terminal\" 2>&1"); - terminal_type = std::regex_replace( - terminal_type, std::regex("[^]*terminal type is ([^ ]+)[^]*"), - "$1"); + terminal_type = run_and_get_output("gnuplot -e \"show terminal\" 2>&1"); + terminal_type = std::regex_replace(terminal_type, + std::regex("[^]*terminal type is ([^ ]+)[^]*"), "$1"); const bool still_dont_know_term_type = terminal_type.empty(); if (still_dont_know_term_type) { terminal_type = "qt"; @@ -337,9 +333,8 @@ namespace matplot::backend { } bool gnuplot::terminal_is_available(std::string_view term) { - std::string msg = - run_and_get_output("gnuplot -e \"set terminal " + - std::string(term.data()) + "\" 2>&1"); + std::string msg = run_and_get_output("gnuplot -e \"set terminal " + + std::string(term.data()) + "\" 2>&1"); return msg.empty(); } diff --git a/source/matplot/backend/gnuplot.h b/source/matplot/backend/gnuplot.h index 17676cbc..dc74abef 100644 --- a/source/matplot/backend/gnuplot.h +++ b/source/matplot/backend/gnuplot.h @@ -7,6 +7,7 @@ #include #include +#include #include #include #include @@ -115,8 +116,8 @@ namespace matplot::backend { } private: - // Pipe to gnuplot process - FILE *pipe_; + // Process pipe to gnuplot + opipe pipe_; // How many bytes we put in the pipe size_t bytes_in_pipe_{0}; diff --git a/source/matplot/util/common.cpp b/source/matplot/util/common.cpp index 2aa2c4f0..9b27f5a9 100644 --- a/source/matplot/util/common.cpp +++ b/source/matplot/util/common.cpp @@ -50,26 +50,10 @@ namespace matplot { iequals(str, "no"); } - struct pipe_deleter { - int operator()(FILE* pipe) const { - if (int status = PCLOSE(pipe); status != -1) - return status; - throw std::system_error{errno, std::system_category(), "pclose"}; - } - }; - std::string run_and_get_output(const std::string &cmd) { - std::unique_ptr pipe(POPEN(cmd.c_str(), "r")); - if (!pipe) { - throw std::system_error{errno, std::system_category(), cmd}; - } - std::array buffer{}; - std::string result; - while (fgets(buffer.data(), static_cast(buffer.size()), - pipe.get()) != nullptr) { - result += buffer.data(); - } - return result; + auto res = std::string{}; + shell_read(cmd, res); + return res; } std::string escape(std::string_view label) { diff --git a/source/matplot/util/popen.cpp b/source/matplot/util/popen.cpp index 7973cbb1..0f8cb869 100644 --- a/source/matplot/util/popen.cpp +++ b/source/matplot/util/popen.cpp @@ -1,102 +1,266 @@ +#define __STDC_WANT_LIB_EXT1__ 1 #include +#include +#include +#include // strerror #ifdef _WIN32 + #include -namespace matplot::detail { - // Function to create a new process and return a FILE pointer for - // input/output. Mimics _popen behaviour, but does not open console windows - // in GUI apps - FILE *hiddenPopen(const char *command, const char *mode) { - SECURITY_ATTRIBUTES saAttr; - HANDLE hChildStdinRd, hChildStdinWr, hStdin, hStdout; - STARTUPINFO si; - PROCESS_INFORMATION pi; - - // Set up security attributes for inheritable handles - saAttr.nLength = sizeof(SECURITY_ATTRIBUTES); - saAttr.bInheritHandle = TRUE; - saAttr.lpSecurityDescriptor = NULL; - - // Create a pipe for the child process's input - if (!CreatePipe(&hChildStdinRd, &hChildStdinWr, &saAttr, 0)) { - return nullptr; - } +int common_pipe::open(const std::string& cmd, char mode) +{ + if (opened()) + close(); // prevent resource leak + HANDLE hChildStdinRd, hChildStdinWr, hStdin, hStdout; + SECURITY_ATTRIBUTES saAttr{}; + // Set up security attributes for inheritable handles + saAttr.nLength = sizeof(SECURITY_ATTRIBUTES); + saAttr.bInheritHandle = TRUE; + saAttr.lpSecurityDescriptor = NULL; - // Ensure the write handle to the pipe is not inherited by child - // processes - if (!SetHandleInformation(hChildStdinWr, HANDLE_FLAG_INHERIT, 0)) { - CloseHandle(hChildStdinRd); - return nullptr; - } + // Create a pipe for the child process's input + if (!CreatePipe(&hChildStdinRd, &hChildStdinWr, &saAttr, 0)) + return report(GetLastError(), "CreatePipe"); - // Create a pipe for the child process's output - if (!CreatePipe(&hStdin, &hStdout, &saAttr, 0)) { - CloseHandle(hChildStdinRd); - CloseHandle(hChildStdinWr); - return nullptr; - } + // Ensure the write handle to the pipe is not inherited by child + // processes + if (!SetHandleInformation(hChildStdinWr, HANDLE_FLAG_INHERIT, 0)) { + auto err = GetLastError(); + CloseHandle(hChildStdinRd); + return report(err, "SetHandleInformation(hChildStdinWr)"); + } - // Ensure the read handle to the output pipe is not inherited by child - // processes - if (!SetHandleInformation(hStdout, HANDLE_FLAG_INHERIT, 0)) { - CloseHandle(hChildStdinRd); - CloseHandle(hChildStdinWr); - CloseHandle(hStdin); - return nullptr; - } + // Create a pipe for the child process's output + if (!CreatePipe(&hStdin, &hStdout, &saAttr, 0)) { + auto err = GetLastError(); + CloseHandle(hChildStdinRd); + CloseHandle(hChildStdinWr); + return report(err, "CreatePipe"); + } - // Configure STARTUPINFO structure for the new process - ZeroMemory(&si, sizeof(STARTUPINFO)); - si.cb = sizeof(STARTUPINFO); - si.hStdError = hStdout; - si.hStdOutput = hStdout; - si.hStdInput = hChildStdinRd; - si.dwFlags |= STARTF_USESTDHANDLES; - - // Create the child process, while hiding the window - if (!CreateProcess(NULL, const_cast(command), NULL, NULL, TRUE, - CREATE_NO_WINDOW, NULL, NULL, &si, &pi)) { - CloseHandle(hChildStdinRd); - CloseHandle(hChildStdinWr); - CloseHandle(hStdin); - CloseHandle(hStdout); - return nullptr; - } + // Ensure the read handle to the output pipe is not inherited by child + // processes + if (!SetHandleInformation(hStdout, HANDLE_FLAG_INHERIT, 0)) { + auto err = GetLastError(); + CloseHandle(hChildStdinRd); + CloseHandle(hChildStdinWr); + CloseHandle(hStdin); + return report(err, "SetHandleInformation(hStdout)"); + } + + // Configure STARTUPINFO structure for the new process + STARTUPINFO si{}; + ZeroMemory(&si, sizeof(STARTUPINFO)); + si.cb = sizeof(STARTUPINFO); + si.hStdError = hStdout; + si.hStdOutput = hStdout; + si.hStdInput = hChildStdinRd; + si.dwFlags |= STARTF_USESTDHANDLES; + PROCESS_INFORMATION pi; - // Close unnecessary handles + // Create the child process, while hiding the window + if (!CreateProcess(NULL, const_cast(cmd.c_str()), NULL, NULL, TRUE, + CREATE_NO_WINDOW, NULL, NULL, &si, &pi)) { + auto err = GetLastError(); CloseHandle(hChildStdinRd); + CloseHandle(hChildStdinWr); + CloseHandle(hStdin); CloseHandle(hStdout); + return report(err, "CreateProcess"); + } + hProcess = pi.hProcess; + hThread = pi.hThread; + + // Close unnecessary handles + CloseHandle(hChildStdinRd); + CloseHandle(hStdout); - // Create a FILE pointer from the write handle to the child process's - // input - FILE *file = _fdopen(_open_osfhandle((intptr_t)hChildStdinWr, 0), mode); + // Create a FILE pointer from the write handle to the child process's + // input + if (mode == 'r') + file_ = _fdopen(_open_osfhandle((intptr_t)hChildStdinWr, 0), "r"); + else + file_ = _fdopen(_open_osfhandle((intptr_t)hChildStdinWr, 0), "w"); + if (file_ == nullptr) { + auto err = GetLastError(); + CloseHandle(hChildStdinWr); + CloseHandle(hStdin); + CloseHandle(pi.hProcess); + CloseHandle(pi.hThread); + return report(err, "_fdopen(_open_osfhandle)"); + } + return 0; +} - if (file == nullptr) { - CloseHandle(hChildStdinWr); - CloseHandle(hStdin); - CloseHandle(pi.hProcess); - CloseHandle(pi.hThread); - return nullptr; +int common_pipe::close(int *exit_code) +{ + if (!opened()) + return 0; // nothing to do + // Close the pipe to process: + fclose(file_); + file_ = nullptr; + // Wait for the process to finish + if (auto r = WaitForSingleObject(hProcess, INFINITE); r != WAIT_OBJECT_0) { + CloseHandle(hThread); + CloseHandle(hProcess); + return ECHILD; + } + // Retrieve the exit code + if (exit_code != nullptr) { + if (BOOL r = GetExitCodeProcess(hProcess, (LPDWORD)exit_code); !r) { + auto err = GetLastError(); + CloseHandle(hThread); + CloseHandle(hProcess); + return err; } + } + CloseHandle(hThread); + CloseHandle(hProcess); + return 0; +} +#endif // _WIN32 implementtion + +#if defined(__linux) || defined(__APPLE__) + +#include +#include // waitpid - return file; +int common_pipe::open(const std::string &cmd, char mode) +{ + if (opened()) + close(); // prevent resource leak + constexpr auto READ = 0u; + constexpr auto WRITE = 1u; + int fd[2]; + if (::pipe(fd) == -1) + return report(errno, "pipe"); + if ((pid = ::fork()) == -1) + return report(errno, "fork"); + + if (pid == 0) { // child process + if (mode == 'r') { + ::close(fd[READ]); // Close the READ end of the pipe + ::dup2(fd[WRITE], 1); // Redirect stdout to pipe + } else { // (mode == 'w') + ::close(fd[WRITE]); // Close the WRITE end of the pipe + dup2(fd[READ], 0); // Redirect stdin to pipe + } + ::setpgid(pid, pid); // Needed so negative PIDs can kill children of /bin/sh + ::execl("/bin/sh", "/bin/sh", "-c", cmd.c_str(), nullptr); + // execl returns only upon error + std::exit(EXIT_FAILURE); + } else { + if (mode == 'r') { + ::close(fd[WRITE]); // Close the WRITE end of the pipe since parent's + // fd is read-only + } else { // (mode == 'w') + ::close(fd[READ]); // Close the READ end of the pipe since parent's fd + // is write-only + } } + if (mode == 'r') + file_ = ::fdopen(fd[READ], "r"); + else + file_ = ::fdopen(fd[WRITE], "w"); + if (file_ == nullptr) + return report(errno, "fdopen"); + return 0; +} - // Function to close the process and retrieve its exit code - int hiddenPclose(FILE *file) { - HANDLE hFile = (HANDLE)_get_osfhandle(_fileno(file)); - fclose(file); - DWORD exitCode; - - // Wait for the process to finish and get its exit code - if (WaitForSingleObject(hFile, INFINITE) == WAIT_OBJECT_0 && - GetExitCodeProcess(hFile, &exitCode)) { - CloseHandle(hFile); - return exitCode; - } else { - return -1; // Failed to get the exit code +/// Closes the pipe opened by popen2 and waits for termination +int common_pipe::close(int *exit_code) +{ + if (!opened()) + return 0; // nothing to do + ::fclose(file_); + file_ = nullptr; + while (::waitpid(pid, exit_code, 0) == -1) { + if (errno != EINTR) { + if (exit_code != nullptr) + *exit_code = -1; + break; } } -} // namespace matplot::detail -#endif // _WIN32 + return 0; +} + +#endif // POSIX implementation + +inline int common_pipe::report(int code, const std::string& what) +{ +#if defined(__STDC_LIB_EXT1__) || defined(_MSC_VER) + char buffer[128]{}; // MSVC has no strerrorlen_s + strerror_s(buffer, 128, code); // MSVC rejects strerror + error_ = what + ": " + buffer; +#else + error_ = what + ": " + std::strerror(code); // GCC fixed strerror and has no strerror_s +#endif // __STDC_LIB_EXT1__ or MSVC + if (exceptions_) + throw std::system_error{code, std::generic_category(), what}; + return code; +} + +ipipe::ipipe() = default; // starting with C++20 could be in-class + +opipe::opipe() = default; // starting with C++20 could be in-class + +int opipe::write(std::string_view data) +{ + constexpr auto CSIZE = sizeof(std::string_view::value_type); + if (!opened()) + return report(EINVAL, "opipe::write"); + if (auto sz = std::fwrite(data.data(), CSIZE, data.length(), file_); + sz != data.size()) { + if (auto err = std::ferror(file_); err != 0) + return report(EIO, "fwrite error"); + if (auto err = std::feof(file_); err != 0) + return report(EIO, "fwrite eof"); + } + return 0; +} + +int opipe::flush(std::string_view data) +{ + if (!opened()) + return report(EINVAL, "opipe::flush"); + if (!data.empty()) + if (auto err = write(data); err != 0) + return report(err, "opipe::write"); + if (auto res = std::fflush(file_); res != 0) + return report(errno, "fflush"); + return 0; +} + +int ipipe::read(std::string &data) +{ + if (!opened()) + return report(EINVAL, "ipipe::read"); + data.clear(); + auto buffer = std::array{}; + while (!std::feof(file_) && !std::ferror(file_)) { + auto count = + std::fread(buffer.data(), sizeof(char), buffer.size(), file_); + if (count > 0) + data.append(buffer.data(), count); + } + if (std::ferror(file_)) + return report(EIO, "fread"); + return 0; +} + +int shell_write(const std::string &cmd, std::string &data) +{ + auto pipe = opipe{}; + if (auto err = pipe.open(cmd); err != 0) + return err; + return pipe.write(data); +} + +int shell_read(const std::string &cmd, std::string &data) +{ + auto pipe = ipipe{}; + if (auto err = pipe.open(cmd); err != 0) + return err; + return pipe.read(data); +} diff --git a/source/matplot/util/popen.h b/source/matplot/util/popen.h index 3d39f332..3318e783 100644 --- a/source/matplot/util/popen.h +++ b/source/matplot/util/popen.h @@ -5,25 +5,91 @@ #ifndef MATPLOTPLUSPLUS_POPEN_H #define MATPLOTPLUSPLUS_POPEN_H +#include +#include +#include // FILE + #ifdef _WIN32 -#include //needed for FILE type -#include -namespace matplot::detail { - // in a GUI application, _popen pops up a window. This version does not. - FILE *hiddenPopen(const char *command, const char *mode); +#include // HANDLE + +/// State of a child process pipe +class proc_pipe +{ +protected: + proc_pipe() = default; + HANDLE hProcess = 0; ///< WIN32 process handle + HANDLE hThread = 0; ///< WIN32 thread handle + FILE *file_ = nullptr; ///< C file handle for I/O (not both) +}; + +#elif defined(__linux) || defined(__APPLE__) - // close the handle opened by hiddenPopen - int hiddenPclose(FILE *file); -} // namespace matplot::detail +#include // pid_t + +/// State of a child process pipe +class proc_pipe +{ +protected: + proc_pipe() = default; + pid_t pid = 0; ///< POSIX process identifier + FILE *file_ = nullptr; ///< C file handle for input or output (not both) +}; -#define PCLOSE ::matplot::detail::hiddenPclose -#define POPEN ::matplot::detail::hiddenPopen -#define FILENO _fileno #else -#define PCLOSE pclose -#define POPEN popen -#define FILENO fileno -#endif +#error "proc_pipe is not implemented for this platform" +#endif // platform specific code + +/// Common operations for a child process pipe +class common_pipe : public proc_pipe +{ +public: + FILE *file() const { return file_; } + int close(int *exit_code = nullptr); + bool opened() const { return file_ != nullptr; } + const std::string& error() const { return error_; } + bool exceptions() const { return exceptions_; } + void exceptions(bool exc) { exceptions_ = exc; } + +protected: + common_pipe() = default; + common_pipe(const common_pipe&) = delete; + common_pipe& operator=(const common_pipe&) = delete; + common_pipe(common_pipe&&) noexcept = default; + common_pipe& operator=(common_pipe&&) noexcept = default; + ~common_pipe() noexcept { + if (opened()) + close(); + } + bool exceptions_ = false; + std::string error_{}; + int report(int err, const std::string& what); + int open(const std::string &command, char mode); +}; + +/// Pipe to read from process output +class ipipe : public common_pipe +{ +public: + ipipe(); + int open(const std::string &cmd) { return common_pipe::open(cmd, 'r'); } + int read(std::string &data); +}; + +/// Pipe to write into process input +class opipe : public common_pipe +{ +public: + opipe(); + int open(const std::string &cmd) { return common_pipe::open(cmd, 'w'); } + int write(std::string_view data); + int flush(std::string_view data = {}); +}; + +/// Runs the shell command, writes data to its input and waits for completion, returns errno if failed +int shell_write(const std::string &command, std::string_view data = {}); + +/// Runs the shell command, reads data from its output and waits for completion, returns errno if failed +int shell_read(const std::string &command, std::string& data); #endif // MATPLOTPLUSPLUS_POPEN_H