From 1e74ce47dc54420ac623f3f2713a8c5b2e20d182 Mon Sep 17 00:00:00 2001 From: Trevor Shoe Date: Tue, 15 Oct 2024 14:39:08 -0400 Subject: [PATCH 01/17] [libssh] Reapply previous version --- 3rd-party/libssh/CMakeLists.txt | 2 +- 3rd-party/libssh/libssh | 2 +- 3rd-party/submodule_info.md | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/3rd-party/libssh/CMakeLists.txt b/3rd-party/libssh/CMakeLists.txt index e778b24ac3..20a3602806 100644 --- a/3rd-party/libssh/CMakeLists.txt +++ b/3rd-party/libssh/CMakeLists.txt @@ -59,7 +59,7 @@ file(STRINGS libssh/CMakeLists.txt libssh_VERSION REGEX "^project\\(libssh") file(STRINGS libssh/CMakeLists.txt LIBRARY_VERSION REGEX "^set\\(LIBRARY_VERSION") file(STRINGS libssh/CMakeLists.txt LIBRARY_SOVERSION REGEX "^set\\(LIBRARY_SOVERSION") -string(REGEX MATCH "^project\\(libssh VERSION ([0-9]+)\\.([0-9]+)\\.([0-9]+) LANGUAGES C\\)$" +string(REGEX MATCH "^project\\(libssh VERSION ([0-9]+)\\.([0-9]+)\\.([0-9]+) LANGUAGES C CXX\\)$" libssh_VERSION "${libssh_VERSION}") if (NOT libssh_VERSION) message(FATAL_ERROR "unable to find libssh project version") diff --git a/3rd-party/libssh/libssh b/3rd-party/libssh/libssh index a09a5a5b5d..f23d1454e5 160000 --- a/3rd-party/libssh/libssh +++ b/3rd-party/libssh/libssh @@ -1 +1 @@ -Subproject commit a09a5a5b5dd414bf422ab3ac3c50626eb46e8d67 +Subproject commit f23d1454e50d0dbb314edd9bf4227ab72303484b diff --git a/3rd-party/submodule_info.md b/3rd-party/submodule_info.md index 4e1aa30c29..eaf19314af 100644 --- a/3rd-party/submodule_info.md +++ b/3rd-party/submodule_info.md @@ -12,7 +12,7 @@ Version: 1.52.1 (+[our patches](https://github.com/CanonicalLtd/grpc/compare/v1. ### libssh -Version: 0.10.6 (+[our patches](https://github.com/canonical/libssh/compare/libssh-0.10.6...multipass)) | +Version: 0.11.1 (+[our patches](https://github.com/canonical/libssh/compare/libssh-0.11.1...multipass)) | | From 6e5b64bf061f626bef3b341083d980e1b82c0952 Mon Sep 17 00:00:00 2001 From: Trevor Shoe Date: Tue, 15 Oct 2024 16:04:35 -0400 Subject: [PATCH 02/17] [ssh] Set timeout to infinite --- include/multipass/ssh/ssh_session.h | 6 +----- src/ssh/ssh_session.cpp | 5 ++--- 2 files changed, 3 insertions(+), 8 deletions(-) diff --git a/include/multipass/ssh/ssh_session.h b/include/multipass/ssh/ssh_session.h index 483eb73124..50223bbc1e 100644 --- a/include/multipass/ssh/ssh_session.h +++ b/include/multipass/ssh/ssh_session.h @@ -33,11 +33,7 @@ class SSHKeyProvider; class SSHSession { public: - SSHSession(const std::string& host, - int port, - const std::string& ssh_username, - const SSHKeyProvider& key_provider, - const std::chrono::milliseconds timeout = std::chrono::seconds(20)); + SSHSession(const std::string& host, int port, const std::string& ssh_username, const SSHKeyProvider& key_provider); // just being explicit (unique_ptr member already caused these to be deleted) SSHSession(const SSHSession&) = delete; diff --git a/src/ssh/ssh_session.cpp b/src/ssh/ssh_session.cpp index 3c3a358a0a..db19c282e1 100644 --- a/src/ssh/ssh_session.cpp +++ b/src/ssh/ssh_session.cpp @@ -33,14 +33,13 @@ namespace mpl = multipass::logging; mp::SSHSession::SSHSession(const std::string& host, int port, const std::string& username, - const SSHKeyProvider& key_provider, - const std::chrono::milliseconds timeout) + const SSHKeyProvider& key_provider) : session{ssh_new(), ssh_free}, mut{} { if (session == nullptr) throw mp::SSHException("could not allocate ssh session"); - const long timeout_secs = std::chrono::duration_cast(timeout).count(); + const long timeout_secs = LONG_MAX; const int nodelay{1}; auto ssh_dir = QDir(MP_STDPATHS.writableLocation(StandardPaths::AppConfigLocation)).filePath("ssh").toStdString(); From 52b54b02dcb83e757d6f01fb25d8ab213a6e0ad0 Mon Sep 17 00:00:00 2001 From: Trevor Shoe Date: Wed, 16 Oct 2024 14:57:12 -0400 Subject: [PATCH 03/17] [ssh] Do not inherit settings from stdin --- src/platform/console/unix_console.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/platform/console/unix_console.cpp b/src/platform/console/unix_console.cpp index 4759893563..19895f921c 100644 --- a/src/platform/console/unix_console.cpp +++ b/src/platform/console/unix_console.cpp @@ -77,7 +77,10 @@ mp::UnixConsole::UnixConsole(ssh_channel channel, UnixTerminal* term) : term{ter term_type = (term_type == nullptr) ? "xterm" : term_type; update_local_pty_size(term->cout_fd()); - ssh_channel_request_pty_size(channel, term_type, local_pty_size.columns, local_pty_size.rows); + + // do not inherit settings from stdin + constexpr unsigned char modes[1] = {0}; + ssh_channel_request_pty_size_modes(channel, term_type, local_pty_size.columns, local_pty_size.rows, modes, sizeof(modes)); } } From 659f1ce6f64b938fed4b674cf823951d9ce37cb2 Mon Sep 17 00:00:00 2001 From: Trevor Shoe Date: Wed, 16 Oct 2024 15:15:02 -0400 Subject: [PATCH 04/17] [ssh] Apply missed formatting --- src/platform/console/unix_console.cpp | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/platform/console/unix_console.cpp b/src/platform/console/unix_console.cpp index 19895f921c..72378f91fc 100644 --- a/src/platform/console/unix_console.cpp +++ b/src/platform/console/unix_console.cpp @@ -80,7 +80,12 @@ mp::UnixConsole::UnixConsole(ssh_channel channel, UnixTerminal* term) : term{ter // do not inherit settings from stdin constexpr unsigned char modes[1] = {0}; - ssh_channel_request_pty_size_modes(channel, term_type, local_pty_size.columns, local_pty_size.rows, modes, sizeof(modes)); + ssh_channel_request_pty_size_modes(channel, + term_type, + local_pty_size.columns, + local_pty_size.rows, + modes, + sizeof(modes)); } } From 70d330921105e6a42478a8e8b708e50a0681182b Mon Sep 17 00:00:00 2001 From: Trevor Shoe Date: Thu, 17 Oct 2024 12:29:27 -0400 Subject: [PATCH 05/17] [ssh] Set stdin to Raw only after libssh inherits sane settings --- src/platform/console/unix_console.cpp | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/src/platform/console/unix_console.cpp b/src/platform/console/unix_console.cpp index 72378f91fc..41c9efd59b 100644 --- a/src/platform/console/unix_console.cpp +++ b/src/platform/console/unix_console.cpp @@ -71,21 +71,15 @@ mp::UnixConsole::UnixConsole(ssh_channel channel, UnixTerminal* term) : term{ter if (term->is_live()) { - setup_console(); - const char* term_type = std::getenv("TERM"); term_type = (term_type == nullptr) ? "xterm" : term_type; update_local_pty_size(term->cout_fd()); - // do not inherit settings from stdin - constexpr unsigned char modes[1] = {0}; - ssh_channel_request_pty_size_modes(channel, - term_type, - local_pty_size.columns, - local_pty_size.rows, - modes, - sizeof(modes)); + ssh_channel_request_pty_size(channel, term_type, local_pty_size.columns, local_pty_size.rows); + + // set stdin to Raw Mode after libssh inherits sane settings from stdin. + setup_console(); } } From 479199fbe88e12865a21d5e045aa525749a29687 Mon Sep 17 00:00:00 2001 From: Trevor Shoe Date: Thu, 17 Oct 2024 17:09:06 -0400 Subject: [PATCH 06/17] [ssh] Fix sshfs process ignoring child not running anymore --- include/multipass/platform.h | 4 +++- src/platform/platform_unix.cpp | 26 +++++++++++++++++++++----- src/ssh/ssh_session.cpp | 2 +- src/sshfs_mount/sshfs_mount.cpp | 18 +++++++++++++++--- src/sshfs_mount/sshfs_mount.h | 4 ++++ src/sshfs_mount/sshfs_server.cpp | 6 ++++-- 6 files changed, 48 insertions(+), 12 deletions(-) diff --git a/include/multipass/platform.h b/include/multipass/platform.h index a5c153d9de..ddfbca5f18 100644 --- a/include/multipass/platform.h +++ b/include/multipass/platform.h @@ -91,7 +91,9 @@ std::unique_ptr make_sshfs_server_process(const SSHFSServerConfig& conf std::unique_ptr make_process(std::unique_ptr&& process_spec); int symlink_attr_from(const char* path, sftp_attributes_struct* attr); -std::function make_quit_watchdog(); // call while single-threaded; call result later, in dedicated thread +std::function make_quit_watchdog( + const std::chrono::milliseconds& timeout, + const std::function& condition); // call while single-threaded; call result later, in dedicated thread std::string reinterpret_interface_id(const std::string& ux_id); // give platforms a chance to reinterpret network IDs diff --git a/src/platform/platform_unix.cpp b/src/platform/platform_unix.cpp index 9e00b6aaf3..d5031db4fc 100644 --- a/src/platform/platform_unix.cpp +++ b/src/platform/platform_unix.cpp @@ -175,11 +175,27 @@ sigset_t mp::platform::make_and_block_signals(const std::vector& sigs) return sigset; } -std::function mp::platform::make_quit_watchdog() +template +timespec make_timespec(std::chrono::duration duration) { - return [sigset = make_and_block_signals({SIGQUIT, SIGTERM, SIGHUP})]() { - int sig = -1; - sigwait(&sigset, &sig); - return sig; + const auto seconds = std::chrono::duration_cast(duration); + + timespec out{}; + out.tv_sec = seconds.count(); + out.tv_nsec = std::chrono::duration_cast(duration - seconds).count(); + return out; +} + +std::function mp::platform::make_quit_watchdog(const std::chrono::milliseconds& timeout, + const std::function& condition) +{ + return [sigset = make_and_block_signals({SIGQUIT, SIGTERM, SIGHUP}), time = make_timespec(timeout), condition]() { + while (condition()) + { + if (const int sig = sigtimedwait(&sigset, nullptr, &time); sig != -1) + return sig; + } + + return SIGCHLD; }; } diff --git a/src/ssh/ssh_session.cpp b/src/ssh/ssh_session.cpp index db19c282e1..91f666e010 100644 --- a/src/ssh/ssh_session.cpp +++ b/src/ssh/ssh_session.cpp @@ -39,7 +39,7 @@ mp::SSHSession::SSHSession(const std::string& host, if (session == nullptr) throw mp::SSHException("could not allocate ssh session"); - const long timeout_secs = LONG_MAX; + const long timeout_secs = std::numeric_limits::max(); const int nodelay{1}; auto ssh_dir = QDir(MP_STDPATHS.writableLocation(StandardPaths::AppConfigLocation)).filePath("ssh").toStdString(); diff --git a/src/sshfs_mount/sshfs_mount.cpp b/src/sshfs_mount/sshfs_mount.cpp index bca8090c90..753c32021b 100644 --- a/src/sshfs_mount/sshfs_mount.cpp +++ b/src/sshfs_mount/sshfs_mount.cpp @@ -151,21 +151,28 @@ auto make_sftp_server(mp::SSHSession&& session, const std::string& source, const } // namespace -mp::SshfsMount::SshfsMount(SSHSession&& session, const std::string& source, const std::string& target, - const mp::id_mappings& gid_mappings, const mp::id_mappings& uid_mappings) - : sftp_server{make_sftp_server(std::move(session), source, target, gid_mappings, uid_mappings)}, +mp::SshfsMount::SshfsMount(SSHSession&& session, + const std::string& source, + const std::string& target, + const mp::id_mappings& gid_mappings, + const mp::id_mappings& uid_mappings) + : running{true}, + sftp_server{make_sftp_server(std::move(session), source, target, gid_mappings, uid_mappings)}, sftp_thread{[this]() { mp::top_catch_all(category, [this] { std::cout << "Connected" << std::endl; sftp_server->run(); std::cout << "Stopped" << std::endl; }); + + running.store(false, std::memory_order_release); }} { } mp::SshfsMount::~SshfsMount() { + running.store(false, std::memory_order_release); stop(); } @@ -175,3 +182,8 @@ void mp::SshfsMount::stop() if (sftp_thread.joinable()) sftp_thread.join(); } + +bool mp::SshfsMount::alive() const +{ + return running.load(std::memory_order_acquire); +} diff --git a/src/sshfs_mount/sshfs_mount.h b/src/sshfs_mount/sshfs_mount.h index bee80681f4..3b037ed174 100644 --- a/src/sshfs_mount/sshfs_mount.h +++ b/src/sshfs_mount/sshfs_mount.h @@ -38,7 +38,11 @@ class SshfsMount void stop(); + [[nodiscard]] + bool alive() const; + private: + std::atomic running; // sftp_server Doesn't need to be a pointer, but done for now to avoid bringing sftp.h // which has an error with -pedantic. std::unique_ptr sftp_server; diff --git a/src/sshfs_mount/sshfs_server.cpp b/src/sshfs_mount/sshfs_server.cpp index 8eb92a5478..0c324d481c 100644 --- a/src/sshfs_mount/sshfs_server.cpp +++ b/src/sshfs_mount/sshfs_server.cpp @@ -105,11 +105,13 @@ int main(int argc, char* argv[]) try { - auto watchdog = mpp::make_quit_watchdog(); // called while there is only one thread - mp::SSHSession session{host, port, username, mp::SSHClientKeyProvider{priv_key_blob}}; mp::SshfsMount sshfs_mount(std::move(session), source_path, target_path, gid_mappings, uid_mappings); + auto watchdog = mpp::make_quit_watchdog(std::chrono::milliseconds{2500}, [&sshfs_mount] { + return sshfs_mount.alive(); + }); // called while there is only one thread + // ssh lives on its own thread, use this thread to listen for quit signal if (int sig = watchdog()) cout << "Received signal " << sig << ". Stopping" << endl; From cd7b9359cb7a915463449ef8aa251ed2638b04c2 Mon Sep 17 00:00:00 2001 From: Trevor Shoe Date: Fri, 18 Oct 2024 10:18:37 -0400 Subject: [PATCH 07/17] [ssh] Treat stfp thread exit as failure --- include/multipass/platform.h | 5 ++--- src/platform/platform_unix.cpp | 9 +++++---- src/sshfs_mount/sshfs_server.cpp | 15 +++++++++------ 3 files changed, 16 insertions(+), 13 deletions(-) diff --git a/include/multipass/platform.h b/include/multipass/platform.h index ddfbca5f18..fd4ba06b2c 100644 --- a/include/multipass/platform.h +++ b/include/multipass/platform.h @@ -91,9 +91,8 @@ std::unique_ptr make_sshfs_server_process(const SSHFSServerConfig& conf std::unique_ptr make_process(std::unique_ptr&& process_spec); int symlink_attr_from(const char* path, sftp_attributes_struct* attr); -std::function make_quit_watchdog( - const std::chrono::milliseconds& timeout, - const std::function& condition); // call while single-threaded; call result later, in dedicated thread +std::function&)> make_quit_watchdog( + const std::chrono::milliseconds& timeout); // call while single-threaded; call result later, in dedicated thread std::string reinterpret_interface_id(const std::string& ux_id); // give platforms a chance to reinterpret network IDs diff --git a/src/platform/platform_unix.cpp b/src/platform/platform_unix.cpp index d5031db4fc..9be4d4c2c3 100644 --- a/src/platform/platform_unix.cpp +++ b/src/platform/platform_unix.cpp @@ -186,16 +186,17 @@ timespec make_timespec(std::chrono::duration duration) return out; } -std::function mp::platform::make_quit_watchdog(const std::chrono::milliseconds& timeout, - const std::function& condition) +std::function&)> mp::platform::make_quit_watchdog( + const std::chrono::milliseconds& timeout) { - return [sigset = make_and_block_signals({SIGQUIT, SIGTERM, SIGHUP}), time = make_timespec(timeout), condition]() { + return [sigset = make_and_block_signals({SIGQUIT, SIGTERM, SIGHUP}), + time = make_timespec(timeout)](const std::function& condition) { while (condition()) { if (const int sig = sigtimedwait(&sigset, nullptr, &time); sig != -1) return sig; } - return SIGCHLD; + return -1; }; } diff --git a/src/sshfs_mount/sshfs_server.cpp b/src/sshfs_mount/sshfs_server.cpp index 0c324d481c..0c1c9d51b7 100644 --- a/src/sshfs_mount/sshfs_server.cpp +++ b/src/sshfs_mount/sshfs_server.cpp @@ -105,19 +105,22 @@ int main(int argc, char* argv[]) try { + auto watchdog = + mpp::make_quit_watchdog(std::chrono::milliseconds{500}); // called while there is only one thread + mp::SSHSession session{host, port, username, mp::SSHClientKeyProvider{priv_key_blob}}; mp::SshfsMount sshfs_mount(std::move(session), source_path, target_path, gid_mappings, uid_mappings); - auto watchdog = mpp::make_quit_watchdog(std::chrono::milliseconds{2500}, [&sshfs_mount] { - return sshfs_mount.alive(); - }); // called while there is only one thread - // ssh lives on its own thread, use this thread to listen for quit signal - if (int sig = watchdog()) + int sig = watchdog([&sshfs_mount] { return sshfs_mount.alive(); }); + + if (sig != -1) cout << "Received signal " << sig << ". Stopping" << endl; + else + cout << "SFTP server thread stopped unexpectedly." << endl; sshfs_mount.stop(); - exit(0); + exit(sig == -1 ? EXIT_FAILURE : EXIT_SUCCESS); } catch (const mp::SSHFSMissingError&) { From cbbe301526c6be13d0f7ae9dd13565675f7bb4e6 Mon Sep 17 00:00:00 2001 From: Trevor Shoe Date: Fri, 18 Oct 2024 12:42:38 -0400 Subject: [PATCH 08/17] [ssh] Formatted [[nodiscard]] --- src/platform/console/unix_console.cpp | 1 - src/sshfs_mount/sshfs_mount.h | 3 +-- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/src/platform/console/unix_console.cpp b/src/platform/console/unix_console.cpp index 41c9efd59b..43e9ee4322 100644 --- a/src/platform/console/unix_console.cpp +++ b/src/platform/console/unix_console.cpp @@ -75,7 +75,6 @@ mp::UnixConsole::UnixConsole(ssh_channel channel, UnixTerminal* term) : term{ter term_type = (term_type == nullptr) ? "xterm" : term_type; update_local_pty_size(term->cout_fd()); - ssh_channel_request_pty_size(channel, term_type, local_pty_size.columns, local_pty_size.rows); // set stdin to Raw Mode after libssh inherits sane settings from stdin. diff --git a/src/sshfs_mount/sshfs_mount.h b/src/sshfs_mount/sshfs_mount.h index 3b037ed174..aa4b0f2fc1 100644 --- a/src/sshfs_mount/sshfs_mount.h +++ b/src/sshfs_mount/sshfs_mount.h @@ -38,8 +38,7 @@ class SshfsMount void stop(); - [[nodiscard]] - bool alive() const; + [[nodiscard]] bool alive() const; private: std::atomic running; From 3631553009453290a6f4ba387acd61881011c44e Mon Sep 17 00:00:00 2001 From: Trevor Shoe Date: Wed, 6 Nov 2024 15:03:17 -0500 Subject: [PATCH 09/17] [sshfs] Use optional for watchdog --- include/multipass/platform.h | 2 +- src/platform/platform_unix.cpp | 8 ++++---- src/sshfs_mount/sshfs_server.cpp | 8 ++++---- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/include/multipass/platform.h b/include/multipass/platform.h index fd4ba06b2c..f34b7c210f 100644 --- a/include/multipass/platform.h +++ b/include/multipass/platform.h @@ -91,7 +91,7 @@ std::unique_ptr make_sshfs_server_process(const SSHFSServerConfig& conf std::unique_ptr make_process(std::unique_ptr&& process_spec); int symlink_attr_from(const char* path, sftp_attributes_struct* attr); -std::function&)> make_quit_watchdog( +std::function(const std::function&)> make_quit_watchdog( const std::chrono::milliseconds& timeout); // call while single-threaded; call result later, in dedicated thread std::string reinterpret_interface_id(const std::string& ux_id); // give platforms a chance to reinterpret network IDs diff --git a/src/platform/platform_unix.cpp b/src/platform/platform_unix.cpp index 9be4d4c2c3..db323e2094 100644 --- a/src/platform/platform_unix.cpp +++ b/src/platform/platform_unix.cpp @@ -186,17 +186,17 @@ timespec make_timespec(std::chrono::duration duration) return out; } -std::function&)> mp::platform::make_quit_watchdog( +std::function(const std::function&)> mp::platform::make_quit_watchdog( const std::chrono::milliseconds& timeout) { return [sigset = make_and_block_signals({SIGQUIT, SIGTERM, SIGHUP}), - time = make_timespec(timeout)](const std::function& condition) { + time = make_timespec(timeout)](const std::function& condition) -> std::optional { while (condition()) { if (const int sig = sigtimedwait(&sigset, nullptr, &time); sig != -1) return sig; } - return -1; + return std::nullopt; }; -} +} \ No newline at end of file diff --git a/src/sshfs_mount/sshfs_server.cpp b/src/sshfs_mount/sshfs_server.cpp index 0c1c9d51b7..eb519a7895 100644 --- a/src/sshfs_mount/sshfs_server.cpp +++ b/src/sshfs_mount/sshfs_server.cpp @@ -112,15 +112,15 @@ int main(int argc, char* argv[]) mp::SshfsMount sshfs_mount(std::move(session), source_path, target_path, gid_mappings, uid_mappings); // ssh lives on its own thread, use this thread to listen for quit signal - int sig = watchdog([&sshfs_mount] { return sshfs_mount.alive(); }); + auto sig = watchdog([&sshfs_mount] { return sshfs_mount.alive(); }); - if (sig != -1) - cout << "Received signal " << sig << ". Stopping" << endl; + if (sig.has_value()) + cout << "Received signal " << *sig << ". Stopping" << endl; else cout << "SFTP server thread stopped unexpectedly." << endl; sshfs_mount.stop(); - exit(sig == -1 ? EXIT_FAILURE : EXIT_SUCCESS); + exit(sig.has_value() ? EXIT_SUCCESS : EXIT_FAILURE); } catch (const mp::SSHFSMissingError&) { From 848a4e52b816b8fc50434a0ac7a2d6f348150091 Mon Sep 17 00:00:00 2001 From: Trevor Shoe Date: Tue, 3 Dec 2024 12:24:36 -0500 Subject: [PATCH 10/17] [sshfs] Use state enum instead of running bool --- include/multipass/platform.h | 3 +++ src/platform/platform_unix.cpp | 8 ++------ src/sshfs_mount/sshfs_mount.cpp | 13 +++++++------ src/sshfs_mount/sshfs_mount.h | 9 ++++++++- 4 files changed, 20 insertions(+), 13 deletions(-) diff --git a/include/multipass/platform.h b/include/multipass/platform.h index f34b7c210f..27f9dbfa47 100644 --- a/include/multipass/platform.h +++ b/include/multipass/platform.h @@ -91,6 +91,9 @@ std::unique_ptr make_sshfs_server_process(const SSHFSServerConfig& conf std::unique_ptr make_process(std::unique_ptr&& process_spec); int symlink_attr_from(const char* path, sftp_attributes_struct* attr); +// Creates a function that will wait for signals or until the passed function returns false. +// The passed function is checked every timeout milliseconds. +// If a signal is received the optional contains it, otherwise the optional is empty. std::function(const std::function&)> make_quit_watchdog( const std::chrono::milliseconds& timeout); // call while single-threaded; call result later, in dedicated thread diff --git a/src/platform/platform_unix.cpp b/src/platform/platform_unix.cpp index db323e2094..f4eec742df 100644 --- a/src/platform/platform_unix.cpp +++ b/src/platform/platform_unix.cpp @@ -179,11 +179,7 @@ template timespec make_timespec(std::chrono::duration duration) { const auto seconds = std::chrono::duration_cast(duration); - - timespec out{}; - out.tv_sec = seconds.count(); - out.tv_nsec = std::chrono::duration_cast(duration - seconds).count(); - return out; + return timespec{seconds.count(), std::chrono::duration_cast(duration - seconds).count()}; } std::function(const std::function&)> mp::platform::make_quit_watchdog( @@ -199,4 +195,4 @@ std::function(const std::function&)> mp::platform::ma return std::nullopt; }; -} \ No newline at end of file +} diff --git a/src/sshfs_mount/sshfs_mount.cpp b/src/sshfs_mount/sshfs_mount.cpp index 753c32021b..ceb3e6c0c3 100644 --- a/src/sshfs_mount/sshfs_mount.cpp +++ b/src/sshfs_mount/sshfs_mount.cpp @@ -156,23 +156,24 @@ mp::SshfsMount::SshfsMount(SSHSession&& session, const std::string& target, const mp::id_mappings& gid_mappings, const mp::id_mappings& uid_mappings) - : running{true}, - sftp_server{make_sftp_server(std::move(session), source, target, gid_mappings, uid_mappings)}, - sftp_thread{[this]() { + : sftp_server{make_sftp_server(std::move(session), source, target, gid_mappings, uid_mappings)}, + sftp_thread{[this] { + state.store(State::Running, std::memory_order_release); + mp::top_catch_all(category, [this] { std::cout << "Connected" << std::endl; sftp_server->run(); std::cout << "Stopped" << std::endl; }); - running.store(false, std::memory_order_release); + state.store(State::Stopped, std::memory_order_release); }} { } mp::SshfsMount::~SshfsMount() { - running.store(false, std::memory_order_release); + state.store(State::Stopped, std::memory_order_release); stop(); } @@ -185,5 +186,5 @@ void mp::SshfsMount::stop() bool mp::SshfsMount::alive() const { - return running.load(std::memory_order_acquire); + return state.load(std::memory_order_acquire) != State::Stopped; } diff --git a/src/sshfs_mount/sshfs_mount.h b/src/sshfs_mount/sshfs_mount.h index aa4b0f2fc1..3799bcd46f 100644 --- a/src/sshfs_mount/sshfs_mount.h +++ b/src/sshfs_mount/sshfs_mount.h @@ -41,7 +41,14 @@ class SshfsMount [[nodiscard]] bool alive() const; private: - std::atomic running; + enum class State + { + Unstarted, + Running, + Stopped + }; + + std::atomic state{State::Unstarted}; // sftp_server Doesn't need to be a pointer, but done for now to avoid bringing sftp.h // which has an error with -pedantic. std::unique_ptr sftp_server; From 3df817facecbc321c5d838bab6e9d996b24ffd53 Mon Sep 17 00:00:00 2001 From: Trevor Shoe Date: Thu, 5 Dec 2024 10:21:54 -0500 Subject: [PATCH 11/17] [sshfs] Change watchdog to not use sigtimedwait --- src/platform/platform_unix.cpp | 31 ++++++++++++++++++++++++++----- 1 file changed, 26 insertions(+), 5 deletions(-) diff --git a/src/platform/platform_unix.cpp b/src/platform/platform_unix.cpp index f4eec742df..895e0d4cf1 100644 --- a/src/platform/platform_unix.cpp +++ b/src/platform/platform_unix.cpp @@ -20,6 +20,7 @@ #include #include +#include #include #include #include @@ -185,14 +186,34 @@ timespec make_timespec(std::chrono::duration duration) std::function(const std::function&)> mp::platform::make_quit_watchdog( const std::chrono::milliseconds& timeout) { - return [sigset = make_and_block_signals({SIGQUIT, SIGTERM, SIGHUP}), + return [sigset = make_and_block_signals({SIGQUIT, SIGTERM, SIGHUP, SIGUSR2}), time = make_timespec(timeout)](const std::function& condition) -> std::optional { - while (condition()) + // create a timer to send SIGUSR2 which unblocks this thread + sigevent sevp{}; + sevp.sigev_notify = SIGEV_SIGNAL; + sevp.sigev_signo = SIGUSR2; + + timer_t timer{}; + if (timer_create(CLOCK_MONOTONIC, &sevp, &timer) == -1) + throw std::runtime_error(fmt::format("Could not create timer: {}", strerror(errno))); + + // scope guard to make sure the timer is deleted once it's no longer needed + const auto timer_guard = sg::make_scope_guard([timer]() noexcept { timer_delete(timer); }); + + // set timer interval and initial time to provided timeout + const itimerspec timer_interval{time, time}; + if (timer_settime(timer, 0, &timer_interval, nullptr) == -1) + throw std::runtime_error(fmt::format("Could not set timer: {}", strerror(errno))); + + // wait on signals and condition + int sig = SIGUSR2; + while (sig == SIGUSR2 && condition()) { - if (const int sig = sigtimedwait(&sigset, nullptr, &time); sig != -1) - return sig; + // can't use sigtimedwait since macOS doesn't support it + sigwait(&sigset, &sig); } - return std::nullopt; + // if sig is SIGUSR2 then we know we're exiting because condition() was false + return sig == SIGUSR2 ? std::nullopt : std::make_optional(sig); }; } From 979ee07c675dad56446ac1f36f9d43a35fe90949 Mon Sep 17 00:00:00 2001 From: Trevor Shoe Date: Fri, 6 Dec 2024 12:19:22 -0500 Subject: [PATCH 12/17] [sshfs] Use seperate signaling thread --- src/platform/platform_unix.cpp | 48 +++++++++++++++++++------------- src/sshfs_mount/sshfs_server.cpp | 2 +- 2 files changed, 30 insertions(+), 20 deletions(-) diff --git a/src/platform/platform_unix.cpp b/src/platform/platform_unix.cpp index 895e0d4cf1..59708b6de6 100644 --- a/src/platform/platform_unix.cpp +++ b/src/platform/platform_unix.cpp @@ -26,6 +26,7 @@ #include #include +#include namespace mp = multipass; @@ -183,37 +184,46 @@ timespec make_timespec(std::chrono::duration duration) return timespec{seconds.count(), std::chrono::duration_cast(duration - seconds).count()}; } +#include + std::function(const std::function&)> mp::platform::make_quit_watchdog( const std::chrono::milliseconds& timeout) { return [sigset = make_and_block_signals({SIGQUIT, SIGTERM, SIGHUP, SIGUSR2}), - time = make_timespec(timeout)](const std::function& condition) -> std::optional { - // create a timer to send SIGUSR2 which unblocks this thread - sigevent sevp{}; - sevp.sigev_notify = SIGEV_SIGNAL; - sevp.sigev_signo = SIGUSR2; - - timer_t timer{}; - if (timer_create(CLOCK_MONOTONIC, &sevp, &timer) == -1) - throw std::runtime_error(fmt::format("Could not create timer: {}", strerror(errno))); + timeout](const std::function& condition) -> std::optional { + std::mutex sig_mtx; + std::condition_variable sig_cv; + int sig = SIGUSR2; - // scope guard to make sure the timer is deleted once it's no longer needed - const auto timer_guard = sg::make_scope_guard([timer]() noexcept { timer_delete(timer); }); + // A signal generator that triggers after `timeout` + AutoJoinThread signaler([&sig_mtx, &sig_cv, &sig, &timeout, signalee = pthread_self()] { + std::unique_lock lock(sig_mtx); + while (sig == SIGUSR2) + { + auto status = sig_cv.wait_for(lock, timeout); - // set timer interval and initial time to provided timeout - const itimerspec timer_interval{time, time}; - if (timer_settime(timer, 0, &timer_interval, nullptr) == -1) - throw std::runtime_error(fmt::format("Could not set timer: {}", strerror(errno))); + if (sig == SIGUSR2 && status == std::cv_status::timeout) + { + pthread_kill(signalee, SIGUSR2); + } + } + }); // wait on signals and condition - int sig = SIGUSR2; - while (sig == SIGUSR2 && condition()) + int ret = SIGUSR2; + while (ret == SIGUSR2 && condition()) { // can't use sigtimedwait since macOS doesn't support it - sigwait(&sigset, &sig); + sigwait(&sigset, &ret); + } + + { + std::unique_lock lock(sig_mtx); + sig = ret == SIGUSR2 ? 0 : ret; } + sig_cv.notify_all(); // if sig is SIGUSR2 then we know we're exiting because condition() was false - return sig == SIGUSR2 ? std::nullopt : std::make_optional(sig); + return ret == SIGUSR2 ? std::nullopt : std::make_optional(sig); }; } diff --git a/src/sshfs_mount/sshfs_server.cpp b/src/sshfs_mount/sshfs_server.cpp index eb519a7895..ee49d7a3e5 100644 --- a/src/sshfs_mount/sshfs_server.cpp +++ b/src/sshfs_mount/sshfs_server.cpp @@ -117,7 +117,7 @@ int main(int argc, char* argv[]) if (sig.has_value()) cout << "Received signal " << *sig << ". Stopping" << endl; else - cout << "SFTP server thread stopped unexpectedly." << endl; + cerr << "SFTP server thread stopped unexpectedly." << endl; sshfs_mount.stop(); exit(sig.has_value() ? EXIT_SUCCESS : EXIT_FAILURE); From 431ff3144d67f579c24caaf4f56fe29a871f489a Mon Sep 17 00:00:00 2001 From: Trevor Shoe Date: Wed, 11 Dec 2024 12:03:11 -0500 Subject: [PATCH 13/17] [libssh] Remove stray include --- src/platform/platform_unix.cpp | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/platform/platform_unix.cpp b/src/platform/platform_unix.cpp index 59708b6de6..9ec49227c1 100644 --- a/src/platform/platform_unix.cpp +++ b/src/platform/platform_unix.cpp @@ -184,8 +184,6 @@ timespec make_timespec(std::chrono::duration duration) return timespec{seconds.count(), std::chrono::duration_cast(duration - seconds).count()}; } -#include - std::function(const std::function&)> mp::platform::make_quit_watchdog( const std::chrono::milliseconds& timeout) { From c5c65a2e6d9713408e8b9cc11b260dd13ae858d3 Mon Sep 17 00:00:00 2001 From: Trevor Shoe Date: Mon, 16 Dec 2024 13:27:00 -0500 Subject: [PATCH 14/17] [sshfs] Address PR reviews --- include/multipass/platform.h | 4 ++-- src/platform/platform_unix.cpp | 17 ++++++----------- 2 files changed, 8 insertions(+), 13 deletions(-) diff --git a/include/multipass/platform.h b/include/multipass/platform.h index 27f9dbfa47..5760d45695 100644 --- a/include/multipass/platform.h +++ b/include/multipass/platform.h @@ -92,10 +92,10 @@ std::unique_ptr make_process(std::unique_ptr&& process_spe int symlink_attr_from(const char* path, sftp_attributes_struct* attr); // Creates a function that will wait for signals or until the passed function returns false. -// The passed function is checked every timeout milliseconds. +// The passed function is checked every `period` milliseconds. // If a signal is received the optional contains it, otherwise the optional is empty. std::function(const std::function&)> make_quit_watchdog( - const std::chrono::milliseconds& timeout); // call while single-threaded; call result later, in dedicated thread + const std::chrono::milliseconds& period); // call while single-threaded; call result later, in dedicated thread std::string reinterpret_interface_id(const std::string& ux_id); // give platforms a chance to reinterpret network IDs diff --git a/src/platform/platform_unix.cpp b/src/platform/platform_unix.cpp index 9ec49227c1..a7c47b191f 100644 --- a/src/platform/platform_unix.cpp +++ b/src/platform/platform_unix.cpp @@ -185,25 +185,20 @@ timespec make_timespec(std::chrono::duration duration) } std::function(const std::function&)> mp::platform::make_quit_watchdog( - const std::chrono::milliseconds& timeout) + const std::chrono::milliseconds& period) { return [sigset = make_and_block_signals({SIGQUIT, SIGTERM, SIGHUP, SIGUSR2}), - timeout](const std::function& condition) -> std::optional { + period](const std::function& condition) -> std::optional { std::mutex sig_mtx; std::condition_variable sig_cv; int sig = SIGUSR2; // A signal generator that triggers after `timeout` - AutoJoinThread signaler([&sig_mtx, &sig_cv, &sig, &timeout, signalee = pthread_self()] { + AutoJoinThread signaler([&sig_mtx, &sig_cv, &sig, &period, signalee = pthread_self()] { std::unique_lock lock(sig_mtx); - while (sig == SIGUSR2) + while (!sig_cv.wait_for(lock, period, [&sig] { return sig != SIGUSR2; })) { - auto status = sig_cv.wait_for(lock, timeout); - - if (sig == SIGUSR2 && status == std::cv_status::timeout) - { - pthread_kill(signalee, SIGUSR2); - } + pthread_kill(signalee, SIGUSR2); } }); @@ -216,7 +211,7 @@ std::function(const std::function&)> mp::platform::ma } { - std::unique_lock lock(sig_mtx); + std::lock_guard lock(sig_mtx); sig = ret == SIGUSR2 ? 0 : ret; } sig_cv.notify_all(); From 6b302312d33e8543e2f42f8a325e41511f99f84d Mon Sep 17 00:00:00 2001 From: Ricardo Abreu Date: Tue, 17 Dec 2024 11:56:47 +0000 Subject: [PATCH 15/17] [unix] Check sigmask return --- src/platform/platform_unix.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/platform/platform_unix.cpp b/src/platform/platform_unix.cpp index a7c47b191f..822de419cb 100644 --- a/src/platform/platform_unix.cpp +++ b/src/platform/platform_unix.cpp @@ -173,7 +173,9 @@ sigset_t mp::platform::make_sigset(const std::vector& sigs) sigset_t mp::platform::make_and_block_signals(const std::vector& sigs) { auto sigset{make_sigset(sigs)}; - pthread_sigmask(SIG_BLOCK, &sigset, nullptr); + if (const auto ec = pthread_sigmask(SIG_BLOCK, &sigset, nullptr); ec) + throw std::runtime_error(fmt::format("Failed to block signals: {}", strerror(ec))); + return sigset; } From 7546f787ab98185e98b152bd0655ea572371bfa9 Mon Sep 17 00:00:00 2001 From: Ricardo Abreu Date: Tue, 17 Dec 2024 11:58:08 +0000 Subject: [PATCH 16/17] [unix] Add const to local variable --- src/platform/platform_unix.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/platform/platform_unix.cpp b/src/platform/platform_unix.cpp index 822de419cb..964c2ec2c2 100644 --- a/src/platform/platform_unix.cpp +++ b/src/platform/platform_unix.cpp @@ -172,7 +172,7 @@ sigset_t mp::platform::make_sigset(const std::vector& sigs) sigset_t mp::platform::make_and_block_signals(const std::vector& sigs) { - auto sigset{make_sigset(sigs)}; + const auto sigset{make_sigset(sigs)}; if (const auto ec = pthread_sigmask(SIG_BLOCK, &sigset, nullptr); ec) throw std::runtime_error(fmt::format("Failed to block signals: {}", strerror(ec))); From f3e3ce0f3698c6bef5929bba0ed856a95e5c47f7 Mon Sep 17 00:00:00 2001 From: Ricardo Abreu Date: Tue, 17 Dec 2024 12:10:00 +0000 Subject: [PATCH 17/17] [unix] Slight include reorderings --- include/multipass/platform_unix.h | 3 +-- src/platform/platform_unix.cpp | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/include/multipass/platform_unix.h b/include/multipass/platform_unix.h index ef82893201..95d306c1bc 100644 --- a/include/multipass/platform_unix.h +++ b/include/multipass/platform_unix.h @@ -18,10 +18,9 @@ #ifndef MULTIPASS_PLATFORM_UNIX_H #define MULTIPASS_PLATFORM_UNIX_H +#include #include -#include - namespace multipass { namespace platform diff --git a/src/platform/platform_unix.cpp b/src/platform/platform_unix.cpp index 964c2ec2c2..f4e2c62f7d 100644 --- a/src/platform/platform_unix.cpp +++ b/src/platform/platform_unix.cpp @@ -14,6 +14,7 @@ * along with this program. If not, see . */ +#include #include #include #include @@ -26,7 +27,6 @@ #include #include -#include namespace mp = multipass;