Rewire VM SSH in preparation for clean-cut VSOCK surgery

include/multipass/ssh/plain_ssh_process.h

@@ -0,0 +1,88 @@
+/*
+ * 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 <http://www.gnu.org/licenses/>.
+ *
+ */
+
+#pragma once
+
+#include <multipass/ssh/ssh_process.h>
+
+#include <libssh/libssh.h>
+
+#include <exception>
+#include <memory>
+#include <mutex>
+#include <variant>
+
+namespace multipass
+{
+class PlainSSHProcess : public SSHProcess
+{
+public:
+    using ChannelUPtr = std::unique_ptr<ssh_channel_struct, void (*)(ssh_channel)>;
+
+    PlainSSHProcess(ssh_session ssh_session,
+                    const std::string& cmd,
+                    std::unique_lock<std::mutex> session_lock);
+
+    // just being explicit (unique_ptr member already caused these to be deleted)
+    PlainSSHProcess(const PlainSSHProcess&) = delete;
+    PlainSSHProcess& operator=(const PlainSSHProcess&) = delete;
+
+    // we should be able to move just fine though
+    PlainSSHProcess(PlainSSHProcess&&) = default;
+    PlainSSHProcess& operator=(PlainSSHProcess&&) = default;
+
+    ~PlainSSHProcess() override = default; // releases session lock
+
+    // Attempt to verify process completion within the given timeout. For this to return true, two
+    // conditions are necessary:
+    //     a) the process did indeed finish;
+    //     b) its exit code is read over ssh within the timeout.
+    //
+    // Note, in particular, that a false return does not guarantee that the process is still
+    // running. It may be just that the exit code was not made available to us in a timely manner.
+    //
+    // This method caches the exit code if we find it, but it keeps the SSHSession locked.
+    bool exit_recognized(
+        std::chrono::milliseconds timeout = std::chrono::milliseconds(10)) override;
+    int exit_code(std::chrono::milliseconds timeout = std::chrono::seconds(5)) override;
+
+    std::string read_std_output() override;
+    std::string read_std_error() override;
+    const std::string& get_cmd() const override;
+
+private:
+    enum class StreamType
+    {
+        out,
+        err
+    };
+
+    void rethrow_if_saved() const;
+    void read_exit_code(std::chrono::milliseconds timeout, bool save_exception);
+    std::string read_stream(StreamType type, int timeout = -1);
+    ssh_channel release_channel(); // releases the lock on the session; callers are on their own to
+                                   // ensure thread safety
+
+    std::unique_lock<std::mutex> session_lock; // do not attempt to re-lock, as this is moved from
+    ssh_session session;
+    std::string cmd;
+    ChannelUPtr channel;
+    std::variant<std::monostate, int, std::exception_ptr> exit_result;
+
+    friend class SftpServer;
+};
+} // namespace multipass

include/multipass/ssh/plain_ssh_session.h

@@ -0,0 +1,64 @@
+/*
+ * 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 <http://www.gnu.org/licenses/>.
+ *
+ */
+
+#pragma once
+
+#include <multipass/ssh/plain_ssh_process.h>
+#include <multipass/ssh/ssh_session.h>
+
+#include <libssh/libssh.h>
+
+#include <memory>
+#include <mutex>
+#include <string>
+
+namespace multipass
+{
+class SSHKeyProvider;
+class PlainSSHSession : public SSHSession
+{
+public:
+    PlainSSHSession(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)
+    PlainSSHSession(const PlainSSHSession&) = delete;
+    PlainSSHSession& operator=(const PlainSSHSession&) = delete;
+
+    // we should be able to move just fine though, but we need to lock
+    PlainSSHSession(PlainSSHSession&&);
+    PlainSSHSession& operator=(PlainSSHSession&&);
+
+    ~PlainSSHSession() override;
+
+    std::unique_ptr<SSHProcess> exec(const std::string& cmd, bool whisper = false) override;
+    [[nodiscard]] bool is_connected() const override;
+
+    operator ssh_session() override;
+    void force_shutdown() override;
+
+private:
+    PlainSSHSession(PlainSSHSession&&, std::unique_lock<std::mutex> lock);
+
+    void set_option(ssh_options_e type, const void* value);
+
+    std::unique_ptr<ssh_session_struct, void (*)(ssh_session)> session;
+    mutable std::mutex mut;
+};
+} // namespace multipass

include/multipass/ssh/ssh_process.h

@@ -17,72 +17,45 @@
 
 #pragma once
 
-#include <libssh/libssh.h>
-
 #include <chrono>
-#include <exception>
-#include <memory>
-#include <mutex>
 #include <string>
-#include <variant>
 
 namespace multipass
 {
 class SSHProcess
 {
 public:
-    using ChannelUPtr = std::unique_ptr<ssh_channel_struct, void (*)(ssh_channel)>;
-
-    SSHProcess(ssh_session ssh_session,
-               const std::string& cmd,
-               std::unique_lock<std::mutex> session_lock);
-
-    // just being explicit (unique_ptr member already caused these to be deleted)
+    virtual ~SSHProcess() = default;
+
+    /**
+     * Check whether the process has finished within the given timeout.
+     * @param timeout Maximum time to wait for completion.
+     * @return @c true if the process finished and its exit code is available; @c false otherwise.
+     * @note A @c false return does not guarantee the process is still running — it may simply mean
+     *       the exit code was not made available in time.
+     */
+    virtual bool exit_recognized(
+        std::chrono::milliseconds timeout = std::chrono::milliseconds(10)) = 0;
+
+    /**
+     * Obtain the exit code of the process, blocking up to the given timeout.
+     * @param timeout Maximum time to wait for the exit code.
+     * @return The process exit code.
+     * @throws ExitlessSSHProcessException if the exit code cannot be obtained within the timeout.
+     */
+    virtual int exit_code(std::chrono::milliseconds timeout = std::chrono::seconds(5)) = 0;
+
+    virtual std::string read_std_output() = 0;
+    virtual std::string read_std_error() = 0;
+    virtual const std::string& get_cmd() const = 0;
+
+protected:
+    SSHProcess() = default;
+
+    // movable but not copyable
     SSHProcess(const SSHProcess&) = delete;
     SSHProcess& operator=(const SSHProcess&) = delete;
-
-    // we should be able to move just fine though
     SSHProcess(SSHProcess&&) = default;
     SSHProcess& operator=(SSHProcess&&) = default;
-
-    ~SSHProcess() = default; // releases session lock
-
-    // Attempt to verify process completion within the given timeout. For this to return true, two
-    // conditions are necessary:
-    //     a) the process did indeed finish;
-    //     b) its exit code is read over ssh within the timeout.
-    //
-    // Note, in particular, that a false return does not guarantee that the process is still
-    // running. It may be just that the exit code was not made available to us in a timely manner.
-    //
-    // This method caches the exit code if we find it, but it keeps the SSHSession locked.
-    bool exit_recognized(
-        std::chrono::milliseconds timeout = std::chrono::milliseconds(10)); // keeps session lock
-    int exit_code(
-        std::chrono::milliseconds timeout = std::chrono::seconds(5)); // releases session lock
-
-    std::string read_std_output();
-    std::string read_std_error();
-
-private:
-    enum class StreamType
-    {
-        out,
-        err
-    };
-
-    void rethrow_if_saved() const;
-    void read_exit_code(std::chrono::milliseconds timeout, bool save_exception);
-    std::string read_stream(StreamType type, int timeout = -1);
-    ssh_channel release_channel(); // releases the lock on the session; callers are on their own to
-                                   // ensure thread safety
-
-    std::unique_lock<std::mutex> session_lock; // do not attempt to re-lock, as this is moved from
-    ssh_session session;
-    std::string cmd;
-    ChannelUPtr channel;
-    std::variant<std::monostate, int, std::exception_ptr> exit_result;
-
-    friend class SftpServer;
 };
 } // namespace multipass

include/multipass/ssh/ssh_session.h

@@ -22,44 +22,28 @@
 #include <libssh/libssh.h>
 
 #include <memory>
-#include <mutex>
 #include <string>
 
 namespace multipass
 {
-class SSHKeyProvider;
 class SSHSession
 {
 public:
-    SSHSession(const std::string& host,
-               int port,
-               const std::string& ssh_username,
-               const SSHKeyProvider& key_provider);
+    virtual ~SSHSession() = default;
 
-    // just being explicit (unique_ptr member already caused these to be deleted)
-    SSHSession(const SSHSession&) = delete;
-    SSHSession& operator=(const SSHSession&) = delete;
-
-    // we should be able to move just fine though, but we need to lock
-    SSHSession(SSHSession&&);
-    SSHSession& operator=(SSHSession&&);
-
-    ~SSHSession();
+    // locks the session until the process is destroyed or exit_code is called!
+    virtual std::unique_ptr<SSHProcess> exec(const std::string& cmd, bool whisper = false) = 0;
+    [[nodiscard]] virtual bool is_connected() const = 0;
 
-    SSHProcess exec(const std::string& cmd,
-                    bool whisper = false); /* locks the session until the process is destroyed
-                                              or exit_code is called! */
-    [[nodiscard]] bool is_connected() const;
+    virtual operator ssh_session() = 0; // careful, not thread safe // TODO@rewiressh drop this?
+    virtual void force_shutdown() = 0;  // careful, not thread safe
 
-    operator ssh_session(); // careful, not thread safe
-    void force_shutdown();  // careful, not thread safe
+protected:
+    SSHSession() = default;
 
-private:
-    SSHSession(SSHSession&&, std::unique_lock<std::mutex> lock);
-
-    void set_option(ssh_options_e type, const void* value);
-
-    std::unique_ptr<ssh_session_struct, void (*)(ssh_session)> session;
-    mutable std::mutex mut;
+    SSHSession(const SSHSession&) = delete;
+    SSHSession& operator=(const SSHSession&) = delete;
+    SSHSession(SSHSession&&) = default;
+    SSHSession& operator=(SSHSession&&) = default;
 };
 } // namespace multipass

include/multipass/utils.h

@@ -21,7 +21,7 @@
 #include <multipass/network_interface_info.h>
 #include <multipass/path.h>
 #include <multipass/singleton.h>
-#include <multipass/ssh/ssh_session.h>
+#include <multipass/ssh/ssh_session.h> // TODO@rewiressh should be fine with fwd decl?
 #include <multipass/virtual_machine.h>
 
 #include <fmt/base.h>
@@ -261,6 +261,7 @@ class Utils : public Singleton<Utils>
     virtual std::string run_in_ssh_session(SSHSession& session,
                                            const std::string& cmd,
                                            bool whisper = false) const;
+    virtual std::string reap_ssh_process(SSHProcess& proc) const;
 
     // various
     virtual std::vector<uint8_t> random_bytes(size_t len);

include/multipass/virtual_machine.h

@@ -35,6 +35,8 @@ namespace multipass
 {
 struct IPAddress;
 class MemorySize;
+class SSHProcess;
+class SSHSession;
 class VMMount;
 struct VMSpecs;
 class MountHandler;
@@ -82,8 +84,12 @@ class VirtualMachine : private DisabledCopyMove
     virtual std::optional<IPAddress> management_ipv4() = 0;
     virtual std::vector<IPAddress> get_all_ipv4() = 0;
 
-    // careful: default param in virtual method; be sure to keep the same value in all descendants
+    // careful: default param in virtual methods; be sure to keep the same value in all descendants
     virtual std::string ssh_exec(const std::string& cmd, bool whisper = false) = 0;
+    virtual std::unique_ptr<SSHProcess> ssh_exec_process(const std::string& cmd,
+                                                         bool whisper = false) = 0;
+
+    [[nodiscard]] virtual std::unique_ptr<SSHSession> new_ssh_session() = 0;
 
     virtual void wait_until_ssh_up(std::chrono::milliseconds timeout) = 0;
     virtual void wait_for_cloud_init(std::chrono::milliseconds timeout) = 0;

src/platform/backends/hyperv/hyperv_virtual_machine.cpp

@@ -24,7 +24,7 @@
 #include <multipass/ip_address.h>
 #include <multipass/logging/log.h>
 #include <multipass/platform.h>
-#include <multipass/ssh/ssh_session.h>
+#include <multipass/ssh/plain_ssh_session.h>
 #include <multipass/top_catch_all.h>
 #include <multipass/utils.h>
 #include <multipass/virtual_machine_description.h>
@@ -58,7 +58,7 @@ std::optional<mp::IPAddress> remote_ip(const std::string& host,
                                        const mp::SSHKeyProvider& key_provider)
 try
 {
-    mp::SSHSession session{host, port, username, key_provider};
+    mp::PlainSSHSession session{host, port, username, key_provider};
 
     sockaddr_in addr{};
     int size = sizeof(addr);