Merge pull request #3978 from canonical/bugfix/sshfs-mount-handler-ensure-signal-disconnect

xmkg · web-flow · commit e932d5f1731c · 2025-04-08T18:12:22.000Z
[sshfs-mount-handler] ensure signals are disconnected upon destruction
diff --git a/include/multipass/qt_delete_later_unique_ptr.h b/include/multipass/qt_delete_later_unique_ptr.h
@@ -31,6 +31,11 @@ namespace multipass
 
 struct QtDeleteLater {
     void operator()(QObject *o) {
+        // Detach the signal handlers since this object doesn't live in a QT object
+        // hierarchy, and that might cause lifetime related issues, especially
+        // if signals are involved.
+        o->disconnect();
+        // Qt requires that a QObject be deleted from the thread it lives in.
         o->deleteLater();
     }
 };
diff --git a/include/multipass/sshfs_mount/sshfs_mount_handler.h b/include/multipass/sshfs_mount/sshfs_mount_handler.h
@@ -36,7 +36,6 @@ class SSHFSMountHandler : public MountHandler
 
     void activate_impl(ServerVariant server, std::chrono::milliseconds timeout) override;
     void deactivate_impl(bool force) override;
-    bool is_active() override;
 
 private:
     qt_delete_later_unique_ptr<Process> process;
diff --git a/src/platform/update/new_release_monitor.cpp b/src/platform/update/new_release_monitor.cpp
@@ -149,6 +149,12 @@ void mp::NewReleaseMonitor::check_for_new_release()
         worker_thread.reset(new mp::LatestReleaseChecker(update_url));
         connect(worker_thread.get(), &mp::LatestReleaseChecker::latest_release_found, this,
                 &mp::NewReleaseMonitor::latest_release_found);
+        // ATTN: The worker_thread is a qt_delete_later_unique_ptr, and the deleter will invoke
+        // disconnect() method upon calling. This is safe to do for this instance because, it's
+        // defined behavior to call disconnect in a signal handler itself. But, it is not without
+        // any quirks. The following signal deliveries might not be triggered if the disconnect happens
+        // in a signal handler. In this particular case, there are none, but be wary of it future travelers.
+        // FIXME: New release monitor code can be much simpler, needs a refactor.
         connect(worker_thread.get(), &QThread::finished, this, [this]() { worker_thread.reset(); });
         worker_thread->start();
     }
diff --git a/src/sshfs_mount/sshfs_mount_handler.cpp b/src/sshfs_mount/sshfs_mount_handler.cpp
@@ -56,7 +56,7 @@ bool has_sshfs(const std::string& name, mp::SSHSession& session)
     // Check if snap support is installed in the instance
     if (session.exec("which snap").exit_code() != 0)
     {
-        mpl::log(mpl::Level::warning, category, fmt::format("Snap support is not installed in '{}'", name));
+        mpl::warn(category, "Snap support is not installed in '{}'", name);
         throw std::runtime_error(
             fmt::format("Snap support needs to be installed in '{}' in order to support mounts.\n"
                         "Please see https://docs.snapcraft.io/installing-snapd for information on\n"
@@ -70,15 +70,14 @@ bool has_sshfs(const std::string& name, mp::SSHSession& session)
     // Check if multipass-sshfs is already installed
     if (session.exec("sudo snap list multipass-sshfs").exit_code(std::chrono::seconds(15)) == 0)
     {
-        mpl::log(mpl::Level::debug, category,
-                 fmt::format("The multipass-sshfs snap is already installed on '{}'", name));
+        mpl::debug(category, "The multipass-sshfs snap is already installed on '{}'", name);
         return true;
     }
 
     // Check if /snap exists for "classic" snap support
     if (session.exec("[ -e /snap ]").exit_code() != 0)
     {
-        mpl::log(mpl::Level::warning, category, fmt::format("Classic snap support symlink is needed in '{}'", name));
+        mpl::warn(category, "Classic snap support symlink is needed in '{}'", name);
         throw std::runtime_error(
             fmt::format("Classic snap support is not enabled for '{}'!\n\n"
                         "Please see https://docs.snapcraft.io/installing-snapd for information on\n"
@@ -92,22 +91,18 @@ bool has_sshfs(const std::string& name, mp::SSHSession& session)
 void install_sshfs_for(const std::string& name, mp::SSHSession& session, const std::chrono::milliseconds& timeout)
 try
 {
-    mpl::log(mpl::Level::info, category, fmt::format("Installing the multipass-sshfs snap in '{}'", name));
+    mpl::info(category, "Installing the multipass-sshfs snap in '{}'", name);
     auto proc = session.exec("sudo snap install multipass-sshfs");
     if (proc.exit_code(timeout) != 0)
     {
         auto error_msg = proc.read_std_error();
-        mpl::log(mpl::Level::error,
-                 category,
-                 fmt::format("Failed to install 'multipass-sshfs': {}", mpu::trim_end(error_msg)));
+        mpl::error(category, "Failed to install 'multipass-sshfs': {}", mpu::trim_end(error_msg));
         throw mp::SSHFSMissingError();
     }
 }
 catch (const mp::ExitlessSSHProcessException& e)
 {
-    mpl::log(mpl::Level::error,
-             category,
-             fmt::format("Could not install 'multipass-sshfs' in '{}': {}", name, e.what()));
+    mpl::error(category, "Could not install 'multipass-sshfs' in '{}': {}", name, e.what());
     throw mp::SSHFSMissingError();
 }
 } // namespace
@@ -130,30 +125,7 @@ SSHFSMountHandler::SSHFSMountHandler(VirtualMachine* vm,
              this->mount_spec.get_gid_mappings(),
              this->mount_spec.get_uid_mappings()}
 {
-    mpl::log(
-        mpl::Level::info,
-        category,
-        fmt::format("initializing mount {} => {} in '{}'", this->mount_spec.get_source_path(), target, vm->vm_name));
-}
-
-bool SSHFSMountHandler::is_active()
-try
-{
-    if (!active || !process || !process->running())
-        return false;
-
-    SSHSession session{vm->ssh_hostname(), vm->ssh_port(), vm->ssh_username(), *ssh_key_provider};
-
-    const auto resolved_target = mp::utils::get_resolved_target(session, target);
-
-    return !session.exec(fmt::format("findmnt --type fuse.sshfs | grep -E '^{} +:{}'", resolved_target, source))
-                .exit_code();
-}
-catch (const std::exception& e)
-{
-    mpl::log(mpl::Level::warning, category,
-             fmt::format("Failed checking SSHFS mount \"{}\" in instance '{}': {}", target, vm->vm_name, e.what()));
-    return false;
+    mpl::info(category, "initializing mount {} => {} in '{}'", this->mount_spec.get_source_path(), target, vm->vm_name);
 }
 
 void SSHFSMountHandler::activate_impl(ServerVariant server, std::chrono::milliseconds timeout)
@@ -177,32 +149,34 @@ void SSHFSMountHandler::activate_impl(ServerVariant server, std::chrono::millise
     config.host = vm->ssh_hostname();
     config.port = vm->ssh_port();
 
-    if (process)
-        process.reset();
     process.reset(platform::make_sshfs_server_process(config).release());
 
     QObject::connect(process.get(), &Process::finished, [this](const ProcessState& exit_state) {
         if (exit_state.completed_successfully())
         {
-            mpl::log(mpl::Level::info, category,
-                     fmt::format("Mount \"{}\" in instance '{}' has stopped", target, vm->vm_name));
+            mpl::info(category, "Mount \"{}\" in instance '{}' has stopped", target, vm->vm_name);
         }
         else
         {
             // not error as it failing can indicate we need to install sshfs in the VM
-            mpl::log(mpl::Level::warning, category,
-                     fmt::format("Mount \"{}\" in instance '{}' has stopped unsuccessfully: {}", target, vm->vm_name,
-                                 exit_state.failure_message()));
+            mpl::warn(category,
+                      "Mount \"{}\" in instance '{}' has stopped unsuccessfully: {}",
+                      target,
+                      vm->vm_name,
+                      exit_state.failure_message());
         }
     });
     QObject::connect(process.get(), &Process::error_occurred, [this](auto error, auto error_string) {
-        mpl::log(mpl::Level::error, category,
-                 fmt::format("There was an error with sshfs_server for instance '{}' with path \"{}\": {} - {}",
-                             vm->vm_name, target, mpu::qenum_to_string(error), error_string));
+        mpl::error(category,
+                   "There was an error with sshfs_server for instance '{}' with path \"{}\": {} - {}",
+                   vm->vm_name,
+                   target,
+                   mpu::qenum_to_string(error),
+                   error_string);
     });
 
-    mpl::log(mpl::Level::info, category, fmt::format("process program '{}'", process->program()));
-    mpl::log(mpl::Level::info, category, fmt::format("process arguments '{}'", process->arguments().join(", ")));
+    mpl::info(category, "process program '{}'", process->program());
+    mpl::info(category, "process arguments '{}'", process->arguments().join(", "));
 
     start_and_block_until_connected(process.get());
     // after the process is started, it must be moved to the main thread
@@ -211,9 +185,10 @@ void SSHFSMountHandler::activate_impl(ServerVariant server, std::chrono::millise
     // when stopping the mount from the main thread again, qt will try to send an event from the main thread to the one
     // in which the process lives this will result in an error since qt can't send events from one thread to another
     process->moveToThread(QCoreApplication::instance()->thread());
+    // So, for any future travelers, this^ is the main reason why we use qt_delete_later_unique_ptr thingy.
 
     // Check in case sshfs_server stopped, usually due to an error
-    auto process_state = process->process_state();
+    const auto process_state = process->process_state();
     if (process_state.exit_code == 9) // Magic number returned by sshfs_server
         throw SSHFSMissingError();
     else if (process_state.exit_code || process_state.error)
@@ -223,18 +198,45 @@ void SSHFSMountHandler::activate_impl(ServerVariant server, std::chrono::millise
 
 void SSHFSMountHandler::deactivate_impl(bool force)
 {
-    mpl::log(mpl::Level::info, category, fmt::format("Stopping mount \"{}\" in instance '{}'", target, vm->vm_name));
+    mpl::info(category, fmt::format("Stopping mount \"{}\" in instance '{}'", target, vm->vm_name));
     QObject::disconnect(process.get(), &Process::error_occurred, nullptr, nullptr);
-    if (process->terminate(); !process->wait_for_finished(5000))
+
+    constexpr auto kProcessWaitTimeout = std::chrono::milliseconds{5000};
+    if (process->terminate(); !process->wait_for_finished(kProcessWaitTimeout.count()))
     {
-        auto err = fmt::format("Failed to terminate SSHFS mount process: {}", process->read_all_standard_error());
+        auto fetch_stderr = [](Process& process) {
+            return fmt::format("Failed to terminate SSHFS mount process gracefully: {}",
+                               process.read_all_standard_error());
+        };
+
+        const auto err = fetch_stderr(*process.get());
+
         if (force)
-            mpl::log(
-                mpl::Level::warning, category,
-                fmt::format("Failed to gracefully stop mount \"{}\" in instance '{}': {}", target, vm->vm_name, err));
+        {
+            mpl::warn(category,
+                      "Failed to gracefully stop mount \"{}\" in instance '{}': {}, trying to stop it forcefully.",
+                      target,
+                      vm->vm_name,
+                      err);
+            /**
+             * Let's try brute force this time.
+             */
+            process->kill();
+            const auto result = process->wait_for_finished(kProcessWaitTimeout.count());
+
+            mpl::warn(category,
+                      "{} to forcefully stop mount \"{}\" in instance '{}': {}",
+                      result ? "Succeeded" : "Failed",
+                      target,
+                      vm->vm_name,
+                      result ? "" : fetch_stderr(*process.get()));
+        }
         else
             throw std::runtime_error{err};
     }
+
+    // Finally, call reset() to disconnect all the signals and defer the deletion
+    // to the owning thread, in this case it's the QT main.
     process.reset();
 }
 
diff --git a/tests/test_sshfs_mount_handler.cpp b/tests/test_sshfs_mount_handler.cpp
@@ -120,9 +120,9 @@ TEST_F(SSHFSMountHandlerTest, mount_creates_sshfs_process)
     factory->register_callback(sshfs_server_callback(sshfs_prints_connected));
 
     mpt::MockVirtualMachine mock_vm{"my_instance"};
-    EXPECT_CALL(mock_vm, ssh_port()).Times(3);
-    EXPECT_CALL(mock_vm, ssh_hostname()).Times(3);
-    EXPECT_CALL(mock_vm, ssh_username()).Times(3);
+    EXPECT_CALL(mock_vm, ssh_port()).Times(2);
+    EXPECT_CALL(mock_vm, ssh_hostname()).Times(2);
+    EXPECT_CALL(mock_vm, ssh_username()).Times(2);
 
     mp::SSHFSMountHandler sshfs_mount_handler{&mock_vm, &key_provider, target_path, mount};
     sshfs_mount_handler.activate(&server);
