Remove chmod and duplicated permissions functions

include/multipass/file_ops.h

@@ -73,15 +73,13 @@ class FileOps : public Singleton<FileOps>
     virtual bool exists(const QFile& file) const;
     virtual bool is_open(const QFile& file) const;
     virtual bool open(QFileDevice& file, QIODevice::OpenMode mode) const;
-    virtual QFileDevice::Permissions permissions(const QFile& file) const;
     virtual qint64 read(QFile& file, char* data, qint64 maxSize) const;
     virtual QByteArray read_all(QFile& file) const;
     virtual QString read_line(QTextStream& text_stream) const;
     virtual bool remove(QFile& file) const;
     virtual bool rename(QFile& file, const QString& newName) const;
     virtual bool resize(QFile& file, qint64 sz) const;
     virtual bool seek(QFile& file, qint64 pos) const;
-    virtual bool setPermissions(QFile& file, QFileDevice::Permissions permissions) const;
     virtual qint64 size(QFile& file) const;
     virtual qint64 write(QFile& file, const char* data, qint64 maxSize) const;
     virtual qint64 write(QFileDevice& file, const QByteArray& data) const;
@@ -112,13 +110,14 @@ class FileOps : public Singleton<FileOps>
     virtual bool remove(const fs::path& path, std::error_code& err) const;
     virtual void create_symlink(const fs::path& to, const fs::path& path, std::error_code& err) const;
     virtual fs::path read_symlink(const fs::path& path, std::error_code& err) const;
-    virtual void permissions(const fs::path& path, fs::perms perms, std::error_code& err) const;
     virtual fs::file_status status(const fs::path& path, std::error_code& err) const;
     virtual fs::file_status symlink_status(const fs::path& path, std::error_code& err) const;
     virtual std::unique_ptr<RecursiveDirIterator> recursive_dir_iterator(const fs::path& path,
                                                                          std::error_code& err) const;
     virtual std::unique_ptr<DirIterator> dir_iterator(const fs::path& path, std::error_code& err) const;
     virtual fs::path weakly_canonical(const fs::path& path) const;
+
+    virtual fs::perms get_permissions(const fs::path& file) const;
 };
 } // namespace multipass
 

include/multipass/platform.h

@@ -58,8 +58,7 @@ class Platform : public Singleton<Platform>
     virtual bool is_remote_supported(const std::string& remote) const;
     virtual bool is_backend_supported(const QString& backend) const; // temporary (?)
     virtual int chown(const char* path, unsigned int uid, unsigned int gid) const;
-    virtual int chmod(const char* path, unsigned int mode) const;
-    virtual bool set_permissions(const multipass::Path path, const QFileDevice::Permissions permissions) const;
+    virtual bool set_permissions(const std::filesystem::path& path, std::filesystem::perms permissions) const;
     virtual bool link(const char* target, const char* link) const;
     virtual bool symlink(const char* target, const char* link, bool is_dir) const;
     virtual int utime(const char* path, int atime, int mtime) const;

include/multipass/utils.h

@@ -28,6 +28,7 @@
 #include <yaml-cpp/yaml.h>
 
 #include <chrono>
+#include <filesystem>
 #include <functional>
 #include <future>
 #include <sstream>
@@ -206,9 +207,10 @@ class Utils : public Singleton<Utils>
     virtual std::string contents_of(const multipass::Path& file_path) const;
     virtual void make_file_with_content(const std::string& file_name, const std::string& content,
                                         const bool& overwrite = false);
-    virtual Path make_dir(const QDir& a_dir, const QString& name,
-                          QFileDevice::Permissions permissions = QFileDevice::Permissions(0));
-    virtual Path make_dir(const QDir& dir, QFileDevice::Permissions permissions = QFileDevice::Permissions(0));
+    virtual Path make_dir(const QDir& a_dir,
+                          const QString& name,
+                          std::filesystem::perms permissions = std::filesystem::perms::none);
+    virtual Path make_dir(const QDir& dir, std::filesystem::perms permissions = std::filesystem::perms::none);
 
     // command and process helpers
     virtual std::string run_cmd_for_output(const QString& cmd, const QStringList& args,

src/daemon/daemon_config.cpp

@@ -110,7 +110,7 @@ std::unique_ptr<const mp::DaemonConfig> mp::DaemonConfigBuilder::build()
 
     auto storage_path = MP_PLATFORM.multipass_storage_location();
     if (!storage_path.isEmpty())
-        MP_UTILS.make_dir(storage_path, QFileDevice::ReadOwner | QFileDevice::WriteOwner | QFileDevice::ExeOwner);
+        MP_UTILS.make_dir(storage_path, std::filesystem::perms::owner_all);
 
     if (cache_directory.isEmpty())
     {

src/platform/platform_linux.cpp

@@ -313,11 +313,10 @@ void mp::platform::Platform::create_alias_script(const std::string& alias, const
 
     MP_UTILS.make_file_with_content(file_path, script, true);
 
-    QFile file(QString::fromStdString(file_path));
     auto permissions =
-        MP_FILEOPS.permissions(file) | QFileDevice::ExeOwner | QFileDevice::ExeGroup | QFileDevice::ExeOther;
+        MP_FILEOPS.get_permissions(file_path) | fs::perms::owner_exec | fs::perms::group_exec | fs::perms::others_exec;
 
-    if (!MP_FILEOPS.setPermissions(file, permissions))
+    if (!MP_PLATFORM.set_permissions(file_path, permissions))
         throw std::runtime_error(fmt::format("cannot set permissions to alias script '{}'", file_path));
 }
 

src/platform/platform_unix.cpp

@@ -58,14 +58,20 @@
     return ::lchown(path, uid, gid);
 }
 
-int mp::platform::Platform::chmod(const char* path, unsigned int mode) const
+bool mp::platform::Platform::set_permissions(const std::filesystem::path& path,
+                                             std::filesystem::perms permissions) const
 {
-    return ::chmod(path, mode);
-}
+    std::error_code ec{};
+    std::filesystem::permissions(path, permissions, ec);
 
-bool mp::platform::Platform::set_permissions(const mp::Path path, const QFileDevice::Permissions permissions) const
-{
-    return QFile::setPermissions(path, permissions);
+    if (ec)
+    {
+        mpl::log(mpl::Level::warning,
+                 "permissions",
+                 fmt::format("failed to set permissions for {}: {}", path.u8string(), ec.message()));
+    }
+
+    return !ec;
 }
 
 bool mp::platform::Platform::symlink(const char* target, const char* link, bool is_dir) const
@@ -99,6 +105,9 @@
 void mp::platform::Platform::set_server_socket_restrictions(const std::string& server_address,
                                                             const bool restricted) const
 {
+    // With C++20 change to using enum
+    using namespace std::filesystem;
+
     auto tokens = mp::utils::split(server_address, ":");
     if (tokens.size() != 2u)
         throw std::runtime_error(fmt::format("invalid server address specified: {}", server_address));
@@ -108,7 +117,7 @@
         return;
 
     int gid{0};
-    int mode{S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP};
+    auto mode{perms::owner_read | perms::owner_write | perms::group_read | perms::group_write};
 
     if (restricted)
     {
@@ -124,16 +133,15 @@
     }
     else
     {
-        mode |= S_IROTH | S_IWOTH;
+        mode |= perms::others_read | perms::others_write;
     }
 
     const auto socket_path = tokens[1];
     if (chown(socket_path.c_str(), 0, gid) == -1)
         throw std::runtime_error(fmt::format("Could not set ownership of the multipass socket: {}", strerror(errno)));
 
-    if (chmod(socket_path.c_str(), mode) == -1)
-        throw std::runtime_error(
-            fmt::format("Could not set permissions for the multipass socket: {}", strerror(errno)));
+    if (!set_permissions(socket_path, mode))
+        throw std::runtime_error(fmt::format("Could not set permissions for the multipass socket"));
 }
 
 QString mp::platform::Platform::multipass_storage_location() const

src/ssh/sftp_client.cpp

@@ -20,6 +20,7 @@
 #include "ssh_client_key_provider.h"
 #include <multipass/file_ops.h>
 #include <multipass/logging/log.h>
+#include <multipass/platform.h>
 #include <multipass/ssh/sftp_utils.h>
 #include <multipass/ssh/throw_on_error.h>
 #include <multipass/utils.h>
@@ -146,9 +147,8 @@ void SFTPClient::pull_file(const fs::path& source_path, const fs::path& target_p
     do_pull_file(source_path, *local_file);
 
     auto source_perms = mp_sftp_stat(sftp.get(), source_path.u8string().c_str())->permissions;
-    std::error_code err;
-    if (MP_FILEOPS.permissions(target_path, static_cast<fs::perms>(source_perms), err); err)
-        throw SFTPError{"cannot set permissions for local file {}: {}", target_path, err.message()};
+    if (!MP_PLATFORM.set_permissions(target_path, static_cast<fs::perms>(source_perms)))
+        throw SFTPError{"cannot set permissions for local file {}", target_path};
 
     if (local_file->fail())
         throw SFTPError{"cannot write to local file {}: {}", target_path, strerror(errno)};
@@ -301,11 +301,11 @@ bool SFTPClient::pull_dir(const fs::path& source_path, const fs::path& target_pa
     for (auto it = subdirectory_perms.crbegin(); it != subdirectory_perms.crend(); ++it)
     {
         const auto& [path, perms] = *it;
-        MP_FILEOPS.permissions(path, static_cast<fs::perms>(perms), err);
-        if (err)
+        if (!MP_PLATFORM.set_permissions(path, static_cast<fs::perms>(perms)))
         {
-            mpl::log(mpl::Level::error, log_category,
-                     fmt::format("cannot set permissions for local directory {}: {}", path, err.message()));
+            mpl::log(mpl::Level::error,
+                     log_category,
+                     fmt::format("cannot set permissions for local directory {}", path));
             success = false;
         }
     }

src/sshfs_mount/sftp_server.cpp

@@ -531,13 +531,11 @@ int mp::SftpServer::handle_mkdir(sftp_client_message msg)
         return reply_failure(msg);
     }
 
-    std::error_code err;
-    MP_FILEOPS.permissions(filename, static_cast<fs::perms>(msg->attr->permissions), err);
-    if (err)
+    if (!MP_PLATFORM.set_permissions(filename, static_cast<fs::perms>(msg->attr->permissions)))
     {
         mpl::log(mpl::Level::trace,
                  category,
-                 fmt::format("{}: set permissions failed for '{}': {}", __FUNCTION__, filename, err.message()));
+                 fmt::format("{}: set permissions failed for '{}'", __FUNCTION__, filename));
         return reply_failure(msg);
     }
 
@@ -964,7 +962,7 @@ int mp::SftpServer::handle_rename(sftp_client_message msg)
 
 int mp::SftpServer::handle_setstat(sftp_client_message msg)
 {
-    QString filename;
+    fs::path filename;
 
     if (sftp_client_message_get_type(msg) == SFTP_FSETSTAT)
     {
@@ -976,70 +974,73 @@ int mp::SftpServer::handle_setstat(sftp_client_message msg)
         }
 
         const auto& [path, _] = *handle;
-        filename = path.string().c_str();
+        filename = path;
     }
     else
     {
         filename = sftp_client_message_get_filename(msg);
-        if (!validate_path(source_path, filename.toStdString()))
+        if (!validate_path(source_path, filename.u8string()))
         {
-            mpl::log(
-                mpl::Level::trace,
-                category,
-                fmt::format("{}: cannot validate path '{}' against source '{}'", __FUNCTION__, filename, source_path));
+            mpl::log(mpl::Level::trace,
+                     category,
+                     fmt::format("{}: cannot validate path '{}' against source '{}'",
+                                 __FUNCTION__,
+                                 filename.u8string(),
+                                 source_path));
             return reply_perm_denied(msg);
         }
 
-        QFileInfo file_info{filename};
+        QFileInfo file_info{QString::fromStdString(filename.u8string())};
         if (!file_info.isSymLink() && !MP_FILEOPS.exists(file_info))
         {
             mpl::log(mpl::Level::trace,
                      category,
-                     fmt::format("{}: cannot setstat '{}': no such file", __FUNCTION__, filename));
+                     fmt::format("{}: cannot setstat '{}': no such file", __FUNCTION__, filename.u8string()));
             return sftp_reply_status(msg, SSH_FX_NO_SUCH_FILE, "no such file");
         }
     }
 
-    QFile file{filename};
-    QFileInfo file_info{filename};
+    QFileInfo file_info{QString::fromStdString(filename.u8string())};
     if (!has_id_mappings_for(file_info))
     {
-        mpl::log(
-            mpl::Level::trace,
-            category,
-            fmt::format("{}: cannot access path \'{}\' without id mapping: permission denied", __FUNCTION__, filename));
+        mpl::log(mpl::Level::trace,
+                 category,
+                 fmt::format("{}: cannot access path \'{}\' without id mapping: permission denied",
+                             __FUNCTION__,
+                             filename.u8string()));
         return reply_perm_denied(msg);
     }
 
     if (msg->attr->flags & SSH_FILEXFER_ATTR_SIZE)
     {
+        QFile file{QString::fromStdString(filename.u8string())};
         if (!MP_FILEOPS.resize(file, msg->attr->size))
         {
-            mpl::log(mpl::Level::trace, category, fmt::format("{}: cannot resize '{}'", __FUNCTION__, filename));
+            mpl::log(mpl::Level::trace,
+                     category,
+                     fmt::format("{}: cannot resize '{}'", __FUNCTION__, filename.u8string()));
             return reply_failure(msg);
         }
     }
 
     if (msg->attr->flags & SSH_FILEXFER_ATTR_PERMISSIONS)
     {
-        std::error_code err;
-        MP_FILEOPS.permissions(file.fileName().toStdString(), static_cast<fs::perms>(msg->attr->permissions), err);
-        if (err)
+        if (!MP_PLATFORM.set_permissions(filename, static_cast<fs::perms>(msg->attr->permissions)))
         {
             mpl::log(mpl::Level::trace,
                      category,
-                     fmt::format("{}: set permissions failed for '{}': {}", __FUNCTION__, filename, err.message()));
+                     fmt::format("{}: set permissions failed for '{}'", __FUNCTION__, filename.u8string()));
             return reply_failure(msg);
         }
     }
 
     if (msg->attr->flags & SSH_FILEXFER_ATTR_ACMODTIME)
     {
-        if (MP_PLATFORM.utime(filename.toStdString().c_str(), msg->attr->atime, msg->attr->mtime) < 0)
+        if (MP_PLATFORM.utime(filename.u8string().c_str(), msg->attr->atime, msg->attr->mtime) < 0)
         {
             mpl::log(mpl::Level::trace,
                      category,
-                     fmt::format("{}: cannot set modification date for '{}'", __FUNCTION__, filename));
+                     fmt::format("{}: cannot set modification date for '{}'", __FUNCTION__, filename.u8string()));
             return reply_failure(msg);
         }
     }
@@ -1050,17 +1051,19 @@ int mp::SftpServer::handle_setstat(sftp_client_message msg)
         {
             mpl::log(mpl::Level::trace,
                      category,
-                     fmt::format("{}: cannot set ownership for \'{}\' without id mapping", __FUNCTION__, filename));
+                     fmt::format("{}: cannot set ownership for \'{}\' without id mapping",
+                                 __FUNCTION__,
+                                 filename.u8string()));
             return reply_perm_denied(msg);
         }
 
-        if (MP_PLATFORM.chown(filename.toStdString().c_str(),
+        if (MP_PLATFORM.chown(filename.u8string().c_str(),
                               reverse_uid_for(msg->attr->uid, msg->attr->uid),
                               reverse_gid_for(msg->attr->gid, msg->attr->gid)) < 0)
         {
             mpl::log(mpl::Level::trace,
                      category,
-                     fmt::format("{}: cannot set ownership for '{}'", __FUNCTION__, filename));
+                     fmt::format("{}: cannot set ownership for '{}'", __FUNCTION__, filename.u8string()));
             return reply_failure(msg);
         }
     }

src/utils/file_ops.cpp

@@ -105,11 +105,6 @@ bool mp::FileOps::open(QFileDevice& file, QIODevice::OpenMode mode) const
     return file.open(mode);
 }
 
-QFileDevice::Permissions mp::FileOps::permissions(const QFile& file) const
-{
-    return file.permissions();
-}
-
 qint64 mp::FileOps::read(QFile& file, char* data, qint64 maxSize) const
 {
     return file.read(data, maxSize);
@@ -145,11 +140,6 @@ bool mp::FileOps::seek(QFile& file, qint64 pos) const
     return file.seek(pos);
 }
 
-bool mp::FileOps::setPermissions(QFile& file, QFileDevice::Permissions permissions) const
-{
-    return file.setPermissions(permissions);
-}
-
 qint64 mp::FileOps::size(QFile& file) const
 {
     return file.size();
@@ -262,11 +252,6 @@ fs::path mp::FileOps::read_symlink(const fs::path& path, std::error_code& err) c
     return fs::read_symlink(path, err);
 }
 
-void mp::FileOps::permissions(const fs::path& path, fs::perms perms, std::error_code& err) const
-{
-    fs::permissions(path, perms, err);
-}
-
 fs::file_status mp::FileOps::status(const fs::path& path, std::error_code& err) const
 {
     return fs::status(path, err);
@@ -292,3 +277,8 @@ fs::path mp::FileOps::weakly_canonical(const fs::path& path) const
 {
     return fs::weakly_canonical(path);
 }
+
+fs::perms mp::FileOps::get_permissions(const fs::path& file) const
+{
+    return fs::status(file).permissions();
+}