From 99428ba28eae17f158067e8577b9f91a324c9e81 Mon Sep 17 00:00:00 2001 From: Robbert van der Helm Date: Tue, 18 May 2021 15:34:35 +0200 Subject: [PATCH] Unify STDIO redirection in Wine host launching This may also fix some weird cases where everything appeared to work fine, but where the file descriptors weren't actually assigned correctly. Not sure what that happened, but it with carla-single the Wine host's STDOUT and STDERR would still point to the orignal pty even though everything still went through the pipes. --- src/plugin/host-process.cpp | 47 ++++--------------------------------- src/plugin/host-process.h | 43 +++++++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+), 43 deletions(-) diff --git a/src/plugin/host-process.cpp b/src/plugin/host-process.cpp index b459c0c6..8ef3264b 100644 --- a/src/plugin/host-process.cpp +++ b/src/plugin/host-process.cpp @@ -18,8 +18,6 @@ #include #include -#include -#include #include #include "src/common/utils.h" @@ -27,40 +25,6 @@ namespace bp = boost::process; namespace fs = boost::filesystem; -/** - * Simple helper function around `boost::process::child` that launches the host - * application (`*.exe`) wrapped in winedbg if compiling with - * `-Dwith-winedbg=true`. Keep in mind that winedbg does not handle arguments - * containing spaces, so most Windows paths will be split up into multiple - * arugments. - */ -template -bp::child launch_host(fs::path host_path, Args&&... args) { - return bp::child( -#ifdef WITH_WINEDBG - // Use the terminal output or `$YABRIDGE_DEBUG_LOG` to get yabridge's - // output, and use the printed target command in a GDB session where you - // load the yabridge `.so` file you're trying to debug - "/usr/bin/winedbg", "--gdb", "--no-start", host_path.string() + ".so", -#else - host_path, -#endif - // NOTE: If the Wine process outlives the host, then it may cause issues - // if our process is still keeping the host's file descriptors - // alive that. This can prevent Ardour from restarting after an - // unexpected shutdown. Because of this we won't use `vfork()`, - // but instead we'll just manually close all non-STDIO file - // descriptors. - bp::extend::on_exec_setup = - [](auto& /*executor*/) { - const int max_fds = static_cast(sysconf(_SC_OPEN_MAX)); - for (int fd = STDERR_FILENO + 1; fd < max_fds; fd++) { - close(fd); - } - }, - std::forward(args)...); -} - HostProcess::HostProcess(boost::asio::io_context& io_context, Logger& logger, Sockets& sockets) @@ -100,12 +64,10 @@ IndividualHost::IndividualHost(boost::asio::io_context& io_context, // watchdog on the Wine plugin host process that shuts down the // sockets after this process shuts down std::to_string(getpid()), - bp::env = plugin_info.create_host_env(), - bp::std_out = stdout_pipe, - bp::std_err = stderr_pipe + bp::env = plugin_info.create_host_env() #ifdef WITH_WINEDBG - , // winedbg has no reliable way to escape spaces, so - // we'll start the process in the plugin's directory + , // winedbg has no reliable way to escape spaces, so + // we'll start the process in the plugin's directory bp::start_dir = plugin_info.windows_plugin_path.parent_path() #endif )) { @@ -189,8 +151,7 @@ GroupHost::GroupHost(boost::asio::io_context& io_context, // it will likely outlive it. bp::child group_host = launch_host(host_path, group_socket_path, - bp::env = plugin_info.create_host_env(), - bp::std_out = stdout_pipe, bp::std_err = stderr_pipe); + bp::env = plugin_info.create_host_env()); group_host.detach(); const pid_t group_host_pid = group_host.id(); diff --git a/src/plugin/host-process.h b/src/plugin/host-process.h index bd776852..4e28c562 100644 --- a/src/plugin/host-process.h +++ b/src/plugin/host-process.h @@ -22,6 +22,8 @@ #include #include #include +#include +#include #include "../common/communication/common.h" #include "../common/logging/common.h" @@ -57,6 +59,47 @@ class HostProcess { */ virtual void terminate() = 0; + /** + * Simple helper function around `boost::process::child` that launches the + * host application (`*.exe`) with some basic setup. This includes setting + * up the asynchronous pipes for STDIO redirection, closing file descriptors + * to prevent leaks, and wrapping everything in winedbg if we're compiling + * with `-Dwith-winedbg=true`. Keep in mind that winedbg does not handle + * arguments containing spaces, so most Windows paths will be split up into + * multiple arugments. + */ + template + boost::process::child launch_host(boost::filesystem::path host_path, + Args&&... args) { + return boost::process::child( +#ifdef WITH_WINEDBG + // Use the terminal output or `$YABRIDGE_DEBUG_LOG` to get + // yabridge's output, and use the printed target command in a GDB + // session where you load the yabridge `.so` file you're trying to + // debug + "/usr/bin/winedbg", "--gdb", "--no-start", + host_path.string() + ".so", +#else + host_path, +#endif + boost::process::std_out = stdout_pipe, + boost::process::std_err = stderr_pipe, + // NOTE: If the Wine process outlives the host, then it may cause + // issues if our process is still keeping the host's file + // descriptors alive that. This can prevent Ardour from + // restarting after an unexpected shutdown. Because of this we + // won't use `vfork()`, but instead we'll just manually close + // all non-STDIO file descriptors. + boost::process::extend::on_exec_setup = + [this](auto& /*executor*/) { + const int max_fds = static_cast(sysconf(_SC_OPEN_MAX)); + for (int fd = STDERR_FILENO + 1; fd < max_fds; fd++) { + close(fd); + } + }, + std::forward(args)...); + } + protected: /** * Initialize the host process by setting up the STDIO redirection.