From 25c3b3888ad7fda1045815bdf58d96280d803e74 Mon Sep 17 00:00:00 2001 From: Trevor Shoe Date: Tue, 15 Oct 2024 14:39:08 -0400 Subject: [PATCH 01/18] [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 8298cafcabc18b1ad0ba77ce6dc8477394fd55ff Mon Sep 17 00:00:00 2001 From: Trevor Shoe Date: Tue, 15 Oct 2024 16:04:35 -0400 Subject: [PATCH 02/18] [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 0e2e120754f612dadc89d5473df13ae6e9ecb172 Mon Sep 17 00:00:00 2001 From: Trevor Shoe Date: Wed, 16 Oct 2024 14:57:12 -0400 Subject: [PATCH 03/18] [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 1da044968b8b7135b20670a6ac5158700db2cb16 Mon Sep 17 00:00:00 2001 From: Trevor Shoe Date: Wed, 16 Oct 2024 15:15:02 -0400 Subject: [PATCH 04/18] [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 bc304c79a085622f5a8c07ca06df547b7e9f0e42 Mon Sep 17 00:00:00 2001 From: Trevor Shoe Date: Thu, 17 Oct 2024 12:29:27 -0400 Subject: [PATCH 05/18] [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 895c11d81794848de184719d7d1f1b71fc2afabd Mon Sep 17 00:00:00 2001 From: Trevor Shoe Date: Thu, 17 Oct 2024 17:09:06 -0400 Subject: [PATCH 06/18] [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 74267c2e9f3b3ad1967b9c7268f94a447fd0a9e5 Mon Sep 17 00:00:00 2001 From: Trevor Shoe Date: Fri, 18 Oct 2024 10:18:37 -0400 Subject: [PATCH 07/18] [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 8e76021251a4ba3a2ffab7076edd16a08832b8f4 Mon Sep 17 00:00:00 2001 From: Trevor Shoe Date: Fri, 18 Oct 2024 12:42:38 -0400 Subject: [PATCH 08/18] [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 e186630c7527565c8482c11db46b7575f9475c7c Mon Sep 17 00:00:00 2001 From: Trevor Shoe Date: Wed, 6 Nov 2024 15:03:17 -0500 Subject: [PATCH 09/18] [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 2cc99744922a7042ab396f222e5000ee221cf987 Mon Sep 17 00:00:00 2001 From: Trevor Shoe Date: Tue, 3 Dec 2024 12:24:36 -0500 Subject: [PATCH 10/18] [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 055d2fe880f9c5b657e0e08de2651b3a48d89b12 Mon Sep 17 00:00:00 2001 From: Trevor Shoe Date: Thu, 5 Dec 2024 10:21:54 -0500 Subject: [PATCH 11/18] [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 c5d9200fbe2226501d12cd87e2a96187aa7a0d21 Mon Sep 17 00:00:00 2001 From: Trevor Shoe Date: Fri, 6 Dec 2024 12:19:22 -0500 Subject: [PATCH 12/18] [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 c9f2ff61b5af2bbf68f80837d5e6a876a5826bda Mon Sep 17 00:00:00 2001 From: Trevor Shoe Date: Wed, 11 Dec 2024 12:03:11 -0500 Subject: [PATCH 13/18] [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 92c8746db88a5797dddef21b9efaaaf3a21887e2 Mon Sep 17 00:00:00 2001 From: Trevor Shoe Date: Mon, 16 Dec 2024 13:27:00 -0500 Subject: [PATCH 14/18] [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 a3e240e0dcceb6af43c6369644ded3d62d4a6fbc Mon Sep 17 00:00:00 2001 From: Trevor Shoe Date: Tue, 17 Dec 2024 09:52:07 -0500 Subject: [PATCH 15/18] [sshfs] Address some review comments --- src/platform/platform_unix.cpp | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/src/platform/platform_unix.cpp b/src/platform/platform_unix.cpp index a7c47b191f..1f0a44ab98 100644 --- a/src/platform/platform_unix.cpp +++ b/src/platform/platform_unix.cpp @@ -20,7 +20,6 @@ #include #include -#include #include #include #include @@ -203,20 +202,20 @@ std::function(const std::function&)> mp::platform::ma }); // wait on signals and condition - int ret = SIGUSR2; - while (ret == SIGUSR2 && condition()) + int latest_signal = SIGUSR2; + while (latest_signal == SIGUSR2 && condition()) { // can't use sigtimedwait since macOS doesn't support it - sigwait(&sigset, &ret); + sigwait(&sigset, &latest_signal); } - { + { // set `sig` to something other than SIGUSR2 so `signaler` knows to exit std::lock_guard lock(sig_mtx); - sig = ret == SIGUSR2 ? 0 : ret; + sig = latest_signal == SIGUSR2 ? 0 : latest_signal; } sig_cv.notify_all(); - // if sig is SIGUSR2 then we know we're exiting because condition() was false - return ret == SIGUSR2 ? std::nullopt : std::make_optional(sig); + // if `sig` is 0 then we know we're exiting because condition() was false + return sig ? std::make_optional(sig) : std::nullopt; }; } From 4d1c44aba438183cbeed45ddf4c5b6e9c9e3e711 Mon Sep 17 00:00:00 2001 From: Trevor Shoe Date: Tue, 17 Dec 2024 13:13:53 -0500 Subject: [PATCH 16/18] [sshfs] Use Timer and wrap posix signals --- include/multipass/platform_unix.h | 21 +++++++++--- src/platform/platform_unix.cpp | 54 ++++++++++++++++++------------- 2 files changed, 49 insertions(+), 26 deletions(-) diff --git a/include/multipass/platform_unix.h b/include/multipass/platform_unix.h index ef82893201..a53cbfc139 100644 --- a/include/multipass/platform_unix.h +++ b/include/multipass/platform_unix.h @@ -22,12 +22,25 @@ #include -namespace multipass +#include "singleton.h" + +#define MP_POSIX_SIGNAL multipass::platform::SignalWrapper::instance() + +namespace multipass::platform { -namespace platform + +class SignalWrapper : public Singleton { +public: + SignalWrapper(const PrivatePass&) noexcept; + + virtual int mask_signals(int how, const sigset_t* sigset, sigset_t* old_set = nullptr) const; + virtual int send(pthread_t target, int signal) const; + virtual int wait(const sigset_t& sigset, int& got) const; +}; + sigset_t make_sigset(const std::vector& sigs); sigset_t make_and_block_signals(const std::vector& sigs); -} // namespace platform -} + +} // namespace multipass::platform #endif // MULTIPASS_PLATFORM_UNIX_H diff --git a/src/platform/platform_unix.cpp b/src/platform/platform_unix.cpp index 1f0a44ab98..ee7343b77d 100644 --- a/src/platform/platform_unix.cpp +++ b/src/platform/platform_unix.cpp @@ -25,7 +25,7 @@ #include #include -#include +#include namespace mp = multipass; @@ -158,6 +158,25 @@ int mp::platform::symlink_attr_from(const char* path, sftp_attributes_struct* at return 0; } +mp::platform::SignalWrapper::SignalWrapper(const PrivatePass& pass) noexcept +: Singleton(pass){} + + +int mp::platform::SignalWrapper::mask_signals(int how, const sigset_t* sigset, sigset_t* old_set) const +{ + return pthread_sigmask(how, sigset, old_set); +} + +int mp::platform::SignalWrapper::send(pthread_t target, int signal) const +{ + return pthread_kill(target, signal); +} + +int mp::platform::SignalWrapper::wait(const sigset_t& sigset, int& got) const +{ + return sigwait(std::addressof(sigset), std::addressof(got)); +} + sigset_t mp::platform::make_sigset(const std::vector& sigs) { sigset_t sigset; @@ -172,7 +191,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)}; - pthread_sigmask(SIG_BLOCK, &sigset, nullptr); + MP_POSIX_SIGNAL.mask_signals(SIG_BLOCK, &sigset, nullptr); return sigset; } @@ -188,34 +207,25 @@ std::function(const std::function&)> mp::platform::ma { return [sigset = make_and_block_signals({SIGQUIT, SIGTERM, SIGHUP, SIGUSR2}), 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, &period, signalee = pthread_self()] { - std::unique_lock lock(sig_mtx); - while (!sig_cv.wait_for(lock, period, [&sig] { return sig != SIGUSR2; })) - { - pthread_kill(signalee, SIGUSR2); - } - }); + + // create a timer to periodically send SIGUSR2 + utils::Timer signal_generator{period, [signalee = pthread_self()] { + MP_POSIX_SIGNAL.send(signalee, SIGUSR2); + }}; // wait on signals and condition int latest_signal = SIGUSR2; while (latest_signal == SIGUSR2 && condition()) { + signal_generator.start(); + // can't use sigtimedwait since macOS doesn't support it - sigwait(&sigset, &latest_signal); + MP_POSIX_SIGNAL.wait(sigset, latest_signal); } - { // set `sig` to something other than SIGUSR2 so `signaler` knows to exit - std::lock_guard lock(sig_mtx); - sig = latest_signal == SIGUSR2 ? 0 : latest_signal; - } - sig_cv.notify_all(); + signal_generator.stop(); - // if `sig` is 0 then we know we're exiting because condition() was false - return sig ? std::make_optional(sig) : std::nullopt; + // if `latest_signal` is SIGUSR2 then we know `condition()` is false + return latest_signal == SIGUSR2 ? std::nullopt : std::make_optional(latest_signal); }; } From 3e8b0e76675cbac55c6487ffe77e8bff60b06a43 Mon Sep 17 00:00:00 2001 From: Trevor Shoe Date: Tue, 17 Dec 2024 14:50:29 -0500 Subject: [PATCH 17/18] [sshfs] Add platform unix tests --- include/multipass/platform_unix.h | 8 +- src/platform/platform_unix.cpp | 11 +-- tests/unix/mock_signal_wrapper.h | 42 +++++++++ tests/unix/test_platform_unix.cpp | 136 ++++++++++++++++++++++++++++++ 4 files changed, 186 insertions(+), 11 deletions(-) create mode 100644 tests/unix/mock_signal_wrapper.h diff --git a/include/multipass/platform_unix.h b/include/multipass/platform_unix.h index a53cbfc139..981330e894 100644 --- a/include/multipass/platform_unix.h +++ b/include/multipass/platform_unix.h @@ -32,11 +32,11 @@ namespace multipass::platform class SignalWrapper : public Singleton { public: - SignalWrapper(const PrivatePass&) noexcept; + SignalWrapper(const PrivatePass&) noexcept; - virtual int mask_signals(int how, const sigset_t* sigset, sigset_t* old_set = nullptr) const; - virtual int send(pthread_t target, int signal) const; - virtual int wait(const sigset_t& sigset, int& got) const; + virtual int mask_signals(int how, const sigset_t* sigset, sigset_t* old_set = nullptr) const; + virtual int send(pthread_t target, int signal) const; + virtual int wait(const sigset_t& sigset, int& got) const; }; sigset_t make_sigset(const std::vector& sigs); diff --git a/src/platform/platform_unix.cpp b/src/platform/platform_unix.cpp index ee7343b77d..2e7c367eb6 100644 --- a/src/platform/platform_unix.cpp +++ b/src/platform/platform_unix.cpp @@ -158,9 +158,9 @@ int mp::platform::symlink_attr_from(const char* path, sftp_attributes_struct* at return 0; } -mp::platform::SignalWrapper::SignalWrapper(const PrivatePass& pass) noexcept -: Singleton(pass){} - +mp::platform::SignalWrapper::SignalWrapper(const PrivatePass& pass) noexcept : Singleton(pass) +{ +} int mp::platform::SignalWrapper::mask_signals(int how, const sigset_t* sigset, sigset_t* old_set) const { @@ -207,11 +207,8 @@ std::function(const std::function&)> mp::platform::ma { return [sigset = make_and_block_signals({SIGQUIT, SIGTERM, SIGHUP, SIGUSR2}), period](const std::function& condition) -> std::optional { - // create a timer to periodically send SIGUSR2 - utils::Timer signal_generator{period, [signalee = pthread_self()] { - MP_POSIX_SIGNAL.send(signalee, SIGUSR2); - }}; + utils::Timer signal_generator{period, [signalee = pthread_self()] { MP_POSIX_SIGNAL.send(signalee, SIGUSR2); }}; // wait on signals and condition int latest_signal = SIGUSR2; diff --git a/tests/unix/mock_signal_wrapper.h b/tests/unix/mock_signal_wrapper.h new file mode 100644 index 0000000000..9701f278a0 --- /dev/null +++ b/tests/unix/mock_signal_wrapper.h @@ -0,0 +1,42 @@ +/* + * Copyright (C) Canonical, Ltd. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 3. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + */ + +#ifndef MULTIPASS_MOCK_SIGNAL_WRAPPER_H +#define MULTIPASS_MOCK_SIGNAL_WRAPPER_H + +#include "../common.h" +#include "../mock_singleton_helpers.h" + +#include + +namespace multipass::test +{ + +class MockSignalWrapper : public platform::SignalWrapper +{ +public: + using SignalWrapper::SignalWrapper; + + MOCK_METHOD(int, mask_signals, (int, const sigset_t*, sigset_t*), (const, override)); + MOCK_METHOD(int, send, (pthread_t, int), (const, override)); + MOCK_METHOD(int, wait, (const sigset_t&, int&), (const, override)); + + MP_MOCK_SINGLETON_BOILERPLATE(MockSignalWrapper, platform::SignalWrapper); +}; + +} // namespace multipass::test + +#endif // MULTIPASS_MOCK_SIGNAL_WRAPPER_H diff --git a/tests/unix/test_platform_unix.cpp b/tests/unix/test_platform_unix.cpp index 02c129e33d..bcc7215582 100644 --- a/tests/unix/test_platform_unix.cpp +++ b/tests/unix/test_platform_unix.cpp @@ -21,6 +21,7 @@ #include #include "mock_libc_functions.h" +#include "mock_signal_wrapper.h" #include #include @@ -142,3 +143,138 @@ TEST_F(TestPlatformUnix, multipassStorageLocationNotSetReturnsEmpty) EXPECT_TRUE(storage_path.isEmpty()); } + +void test_sigset_empty(const sigset_t& set) +{ + // there is no standard empty check to try a few different signals + EXPECT_EQ(sigismember(&set, SIGABRT), 0); + EXPECT_EQ(sigismember(&set, SIGALRM), 0); + EXPECT_EQ(sigismember(&set, SIGCHLD), 0); + EXPECT_EQ(sigismember(&set, SIGINT), 0); + EXPECT_EQ(sigismember(&set, SIGSEGV), 0); + EXPECT_EQ(sigismember(&set, SIGKILL), 0); + EXPECT_EQ(sigismember(&set, SIGQUIT), 0); + EXPECT_EQ(sigismember(&set, SIGTERM), 0); + EXPECT_EQ(sigismember(&set, SIGUSR1), 0); + EXPECT_EQ(sigismember(&set, SIGUSR2), 0); +} + +bool test_sigset_has(const sigset_t& set, const std::vector& sigs) +{ + bool good = true; + for (auto sig : sigs) + { + auto has_sig = sigismember(&set, sig) == 1; + if (!has_sig) + good = false; + + EXPECT_TRUE(has_sig); + } + + return good; +} + +TEST_F(TestPlatformUnix, make_sigset_returns_emptyset) +{ + auto set = mp::platform::make_sigset({}); + test_sigset_empty(set); +} + +TEST_F(TestPlatformUnix, make_sigset_makes_sigset) +{ + auto set = mp::platform::make_sigset({SIGINT, SIGUSR2}); + + // check signals are set + test_sigset_has(set, {SIGINT, SIGUSR2}); + + // unset set signals + sigdelset(&set, SIGUSR2); + sigdelset(&set, SIGINT); + + // check other signals aren't set + test_sigset_empty(set); +} + +TEST_F(TestPlatformUnix, make_and_block_signals_works) +{ + auto [mock_signals, guard] = mpt::MockSignalWrapper::inject(); + + EXPECT_CALL( + *mock_signals, + mask_signals(SIG_BLOCK, Pointee(Truly([](const auto& set) { return test_sigset_has(set, {SIGINT}); })), _)); + + auto set = mp::platform::make_and_block_signals({SIGINT}); + + test_sigset_has(set, {SIGINT}); + + sigdelset(&set, SIGINT); + test_sigset_empty(set); +} + +TEST_F(TestPlatformUnix, make_quit_watchdog_blocks_signals) +{ + auto [mock_signals, guard] = mpt::MockSignalWrapper::inject(); + + EXPECT_CALL(*mock_signals, + mask_signals(SIG_BLOCK, + Pointee(Truly([](const auto& set) { + return test_sigset_has(set, {SIGQUIT, SIGTERM, SIGHUP, SIGUSR2}); + })), + _)); + + mp::platform::make_quit_watchdog(std::chrono::milliseconds{1}); +} + +TEST_F(TestPlatformUnix, quit_watchdog_quits_on_condition) +{ + auto [mock_signals, guard] = mpt::MockSignalWrapper::inject(); + + EXPECT_CALL(*mock_signals, mask_signals(SIG_BLOCK, _, _)); + EXPECT_CALL(*mock_signals, wait(_, _)).WillRepeatedly(DoAll(SetArgReferee<1>(SIGUSR2), Return(0))); + ON_CALL(*mock_signals, send(pthread_self(), SIGUSR2)).WillByDefault(Return(0)); + + auto watchdog = mp::platform::make_quit_watchdog(std::chrono::milliseconds{1}); + EXPECT_EQ(watchdog([] { return false; }), std::nullopt); +} + +TEST_F(TestPlatformUnix, quit_watchdog_quits_on_signal) +{ + auto [mock_signals, guard] = mpt::MockSignalWrapper::inject(); + + EXPECT_CALL(*mock_signals, mask_signals(SIG_BLOCK, _, _)); + EXPECT_CALL(*mock_signals, wait(_, _)) + .WillOnce(DoAll(SetArgReferee<1>(SIGUSR2), Return(0))) + .WillOnce(DoAll(SetArgReferee<1>(SIGTERM), Return(0))); + ON_CALL(*mock_signals, send(pthread_self(), SIGUSR2)).WillByDefault(Return(0)); + + auto watchdog = mp::platform::make_quit_watchdog(std::chrono::milliseconds{1}); + EXPECT_EQ(watchdog([] { return true; }), SIGTERM); +} + +TEST_F(TestPlatformUnix, quit_watchdog_signals_itself_asynchronously) +{ + auto [mock_signals, guard] = mpt::MockSignalWrapper::inject(); + + std::atomic signaled = false; + std::atomic times = 0; + + EXPECT_CALL(*mock_signals, mask_signals(SIG_BLOCK, _, _)); + EXPECT_CALL(*mock_signals, wait(_, _)) + .WillRepeatedly(DoAll( + [&signaled, ×] { + while (!signaled.load(std::memory_order_acquire)) + { + } + times.fetch_add(1, std::memory_order_release); + signaled.store(false, std::memory_order_release); + }, + SetArgReferee<1>(SIGUSR2), + Return(0))); + + EXPECT_CALL(*mock_signals, send(pthread_self(), SIGUSR2)) + .WillRepeatedly(DoAll([&signaled] { signaled.store(true, std::memory_order_release); }, Return(0))); + + auto watchdog = mp::platform::make_quit_watchdog(std::chrono::milliseconds{1}); + EXPECT_EQ(watchdog([×] { return times.load(std::memory_order_acquire) < 10; }), std::nullopt); + EXPECT_GE(times.load(std::memory_order_acquire), 10); +} From ada20cefc5c9e7131583d9e6272aaaa98a30ee7e Mon Sep 17 00:00:00 2001 From: Trevor Shoe Date: Tue, 17 Dec 2024 16:50:44 -0500 Subject: [PATCH 18/18] [sshfs] Yield and move include --- src/platform/platform_unix.cpp | 2 +- tests/unix/test_platform_unix.cpp | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/src/platform/platform_unix.cpp b/src/platform/platform_unix.cpp index 2e7c367eb6..d3e02d01eb 100644 --- a/src/platform/platform_unix.cpp +++ b/src/platform/platform_unix.cpp @@ -17,6 +17,7 @@ #include #include #include +#include #include #include @@ -25,7 +26,6 @@ #include #include -#include namespace mp = multipass; diff --git a/tests/unix/test_platform_unix.cpp b/tests/unix/test_platform_unix.cpp index bcc7215582..3a4cc69028 100644 --- a/tests/unix/test_platform_unix.cpp +++ b/tests/unix/test_platform_unix.cpp @@ -27,6 +27,8 @@ #include #include +#include + namespace mp = multipass; namespace mpt = multipass::test; @@ -262,8 +264,10 @@ TEST_F(TestPlatformUnix, quit_watchdog_signals_itself_asynchronously) EXPECT_CALL(*mock_signals, wait(_, _)) .WillRepeatedly(DoAll( [&signaled, ×] { + // busy wait until signaled while (!signaled.load(std::memory_order_acquire)) { + std::this_thread::yield(); } times.fetch_add(1, std::memory_order_release); signaled.store(false, std::memory_order_release);