Replace Qt with std in critical parts of sftp_server #3270

andrei-toterman · 2023-10-24T11:04:36Z

No description provided.

codecov · 2023-10-24T13:23:30Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (8bc1e83) 83.91% compared to head (147abc0) 83.90%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3270      +/-   ##
==========================================
- Coverage   83.91%   83.90%   -0.02%     
==========================================
  Files         250      250              
  Lines       13526    13547      +21     
==========================================
+ Hits        11351    11367      +16     
- Misses       2175     2180       +5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

townsend2010

Hey @andrei-toterman!

Thanks for this! Unfortunately, it is not working for me. If I create a mount, shell into the instance, cd to the mounted directory, I get the following:

$ git clone https://github.com/canonical/multipass.git
Cloning into 'multipass'...
error: Unable to create '/home/ubuntu/test/multipass/.git/HEAD.lock': Operation not permitted

Also, I haven't full reviewed, but I did leave one comment below.

src/sshfs_mount/sftp_server.cpp

georgeliao · 2023-11-28T14:59:33Z

src/sshfs_mount/sftp_server.cpp

+template <typename T>
+T* multipass::SftpServer::get_handle(sftp_client_message msg)
+{
+    return reinterpret_cast<T*>(sftp_handle(msg->sftp, msg->handle));


Maybe using static_cast which has some compile time checks instead of reinterpret_cast.

Yup, makes sense.

georgeliao · 2023-11-29T09:23:14Z

include/multipass/recursive_dir_iterator.h

+    std::optional<fs::path> self;
+    std::optional<fs::path> parent;
+    fs::directory_iterator iter;
+    DirectoryEntry current;


Nice wrapper utility class, besides that, I have a few small comments.

About the data member current, it holds the same information as *iter. I think you did this double bookkeeping because you need to return DirectoryEntry reference. I am wondering if it is gotta be better if you return by value and remove all the current related code.

There are some implicit constructions from fs::directory_entry to DirectoryEntry, maybe it is better to change the constructor DirectoryEntry(const fs::directory_entry& _entry) to explicit and explicitly convert these.

You are right, current is necessary for returning a DirectoryEntry reference. But the reason it returns a reference instead of a value is so that it can return a MockDirectoryEntry reference when writing unit tests.
As for the explicit constructors, sure, I will convert them.

georgeliao · 2023-11-29T09:58:18Z

src/sshfs_mount/sftp_server.h

    SSHSession ssh_session;
    SSHFSProcUptr sshfs_process;
    SftpSessionUptr sftp_server_session;
    const std::string source_path;
    const std::string target_path;
-    std::unordered_map<void*, std::unique_ptr<QFileInfoList>> open_dir_handles;
-    std::unordered_map<void*, std::unique_ptr<QFile>> open_file_handles;
+    std::unordered_map<void*, std::unique_ptr<NamedFd, void (*)(NamedFd*)>> open_file_handles;


Maybe change from void (*)(NamedFd*) to std::function<void(NamedFd*)>, so we can have a more readable functor signature. The same goes to other occurrences of void (*)(NamedFd*).

Good call. At first I avoided std::function because I thought it always involves a dynamic allocation, but after reading the docs better, I understand that in the case of free function pointers, small object optimization is guaranteed and no dynamic allocation takes place. So I'll change it.

georgeliao · 2023-11-29T11:29:26Z

src/utils/file_ops.cpp

+    const auto named_fd = new NamedFd{path, fd};
+    const auto deleter = [](NamedFd* named_fd) {
+        if (named_fd->second != -1)
+            ::close(named_fd->second);


I think it is the responsibility of the custom deleter to delete the memory explicitly. So a line delete named_fd; will have to be added after the if branch.

if (named_fd->second != -1) ::close(named_fd->second); delete named_fd;

Besides that, maybe move the new NamedFd{path, fd}; into the return statement that makes return {new NamedFd{path, fd}, deleter}; and remove the line const auto named_fd = new NamedFd{path, fd};.

Oh, really good catch!

Given that I made this mistake regarding the destructor, and the std::fucntion over simple function pointer comment, do you think that it would be worth it to change NamedFd from a simple std::pair to a proper struct that has its own destructor? That way, we can use a simple std::unique_ptr<NamedFd> and not worry about providing a correct deleter. Initially I used std::pair because I wanted to keep things simple, but maybe they are not as simple as I thought.

Actually, that is an even better idea. Having a proper struct with the proper RAII and avoiding all the custom deleter hassle.

georgeliao · 2023-11-29T12:04:27Z

src/sshfs_mount/sftp_server.cpp

-    QFile file(filename);
-    if (!MP_FILEOPS.setPermissions(file, to_qt_permissions(msg->attr->permissions)))
+    std::error_code err;
+    MP_FILEOPS.permissions(filename, static_cast<fs::perms>(msg->attr->permissions), err);


Maybe static_cast<fs::perms>(msg->attr->permissions) -> fs::perms(msg->attr->permissions)? both interpret uint32_t into bitmask, but the second one is cleaner.

Hey guys, sorry I noticed this flying by. Why is a functional cast here preferable to a static_cast?

My take would be that they both rely on the underneath type conversion, but the direct cast is cleaner.

Perhaps I am missing something in this case, but to me it looks like this would apply: https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Res-casts-named

I guess {} is OK too if no narrowing is involved.

Yes, I think {} is the cleanest. I understand that a C-style cast is not really recommended and, if this was a conversion between more complex data types i.e. classes, I would also definitely prefer named casts. In this case, since it's just an integer to enum conversion, I initially agreed to the C-style cast since it would simply act like the static_cast that it replaced. But now that you mention {}, I think that is preferable. Thanks for pointing it out!

I guess {} is OK too if no narrowing is involved.

That is what the item of the guideline said. On the top of that, () and {} functional cast are also slightly different in terms of narrowing prevention.

Yes, I think {} is the cleanest. I understand that a C-style cast is not really recommended and,

I think you mean functional cast, C-style is the (T) var based on the guideline.

Well, unfortunately using {} doesn't compile on windows, so I'll just go back to the static_cast for correctness.

Ahaa! And thus we just prevented a narrowing conversion.

georgeliao · 2023-11-29T14:02:40Z

src/sshfs_mount/sftp_server.cpp

        {
-            mp::platform::symlink_attr_from(entry.absoluteFilePath().toStdString().c_str(), &attr);
+            std::error_code _;


This _ variable is not used. I think.

True. Must've forgotten it there from a previous attempt.

georgeliao · 2023-11-29T14:29:51Z

tests/test_sftpserver.cpp

    msg->filename = dir_name.data();

-    int perm_denied_num_calls{0};
-    auto reply_status = make_reply_status(msg.get(), SSH_FX_PERMISSION_DENIED, perm_denied_num_calls);
+    auto [file_ops, mock_file_ops_guard] = mpt::MockFileOps::inject();


The pair can be const, maybe do a auto [file_ops, mock_file_ops_guard] = mpt::MockFileOps::inject(); -> const auto [file_ops, mock_file_ops_guard] = mpt::MockFileOps::inject(); string replacement in the whole file.

georgeliao · 2023-11-29T15:56:29Z

Hi @andrei-toterman , excellent work on the refactoring and some utility function addition. Only a few comments need to be processed and most of them are small ones.

georgeliao · 2023-11-30T14:03:17Z

@andrei-toterman , I have done the functional testing on various platforms and backends. The testing includes mounting the folder via classic mount and using the folder for creating/deleting files, git clone repo and delete the cloned folder, etc. They all work fine. @townsend2010 , I could not reproduce your permission denied error. Maybe that was based on the old version of the code.

townsend2010 · 2023-11-30T14:04:49Z

Hi @georgeliao! Yes, that issue I ran into was with an older version of this branch and I have verified that it is now fixed as well. Thanks!

townsend2010 · 2023-11-30T14:07:44Z

Hey @andrei-toterman,

Just a question about test coverage...this branch is not very well covered. Is this due to a difficulty in testing the actual changes or is it an oversight?

andrei-toterman · 2023-11-30T14:34:07Z

Hey, @townsend2010! Most of the uncovered code comes from wrapper functions for file IO and filesystem information. I could write some tests for those, but they would probably look like "make sure that the wrapper function does the same thing as the wrapped function". But I agree that it's not nice to decrease the coverage, so I will add some simple tests for those. As for the uncovered lines from the sftp server code, that is indeed an oversight on my part, and I'll fix it!

georgeliao · 2023-12-04T10:20:56Z

tests/test_file_ops.cpp

@@ -19,6 +19,9 @@

 #include <multipass/file_ops.h>

+#include <fcntl.h>
+#include <streambuf>


I do not think we need the header streambuf.

georgeliao · 2023-12-05T11:32:53Z

@andrei-toterman I have done another round of functional testing. It includes qemu and lxd on linux; Hyper-v and VirtualBox on virtualized windows; qemu on macOs. They all work fine. I think now it is ready to merge. @townsend2010 can you also remove the change request?

townsend2010

LGTM now too, thanks!

andrei-toterman force-pushed the sftp-server-std-over-qt branch from 9fb684a to 92464e9 Compare October 24, 2023 12:02

andrei-toterman force-pushed the sftp-server-std-over-qt branch 3 times, most recently from 9153913 to b279dc6 Compare October 26, 2023 10:39

andrei-toterman force-pushed the sftp-server-std-over-qt branch from b279dc6 to 4163ea5 Compare November 3, 2023 14:03

townsend2010 suggested changes Nov 3, 2023

View reviewed changes

src/sshfs_mount/sftp_server.cpp Show resolved Hide resolved

townsend2010 linked an issue Nov 6, 2023 that may be closed by this pull request

This is an exact duplicate of https://github.com/canonical/multipass/issues/71 that was never solved or resolved #3174

Closed

andrei-toterman force-pushed the sftp-server-std-over-qt branch 4 times, most recently from 6eaae09 to 17f8756 Compare November 10, 2023 07:11

andrei-toterman linked an issue Nov 10, 2023 that may be closed by this pull request

Rewrite SFTP server code using std::filesystem #3246

Closed

andrei-toterman force-pushed the sftp-server-std-over-qt branch 5 times, most recently from 0a16dfd to c2bec0d Compare November 16, 2023 11:39

townsend2010 requested a review from georgeliao November 28, 2023 13:31

georgeliao reviewed Nov 28, 2023

View reviewed changes

georgeliao reviewed Nov 29, 2023

View reviewed changes

andrei-toterman force-pushed the sftp-server-std-over-qt branch 2 times, most recently from 714457c to 64519b6 Compare November 30, 2023 13:43

andrei-toterman added 2 commits December 1, 2023 11:55

[fileops] add non-recursive dir iterator + other file ops utils

9ca9054

make DirectoryEntry constructor explicit

4548b5a

andrei-toterman force-pushed the sftp-server-std-over-qt branch 2 times, most recently from 7735da5 to 2a4a538 Compare December 1, 2023 14:30

andrei-toterman added 3 commits December 1, 2023 16:12

[sftp] rewrite some parts of sftp server to use std over qt

e8b16b1

[tests] re-write some sftp server tests

01144ca

[tests/sftp] mark mocked singletons as const

a9381cf

andrei-toterman force-pushed the sftp-server-std-over-qt branch from 2a4a538 to a41f57d Compare December 1, 2023 15:14

andrei-toterman requested a review from georgeliao December 1, 2023 18:21

georgeliao reviewed Dec 4, 2023

View reviewed changes

[tests/fileops] add tests for new utility methods

147abc0

andrei-toterman force-pushed the sftp-server-std-over-qt branch from a41f57d to 147abc0 Compare December 4, 2023 10:44

georgeliao approved these changes Dec 5, 2023

View reviewed changes

townsend2010 approved these changes Dec 5, 2023

View reviewed changes

townsend2010 merged commit 102cd1d into main Dec 5, 2023
13 of 14 checks passed

townsend2010 deleted the sftp-server-std-over-qt branch December 5, 2023 14:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace Qt with std in critical parts of sftp_server #3270

Replace Qt with std in critical parts of sftp_server #3270

andrei-toterman commented Oct 24, 2023

codecov bot commented Oct 24, 2023 •

edited

Loading

townsend2010 left a comment

georgeliao Nov 28, 2023

andrei-toterman Nov 29, 2023

georgeliao Nov 29, 2023

andrei-toterman Nov 29, 2023

georgeliao Nov 29, 2023 •

edited

Loading

andrei-toterman Nov 29, 2023

georgeliao Nov 29, 2023

andrei-toterman Nov 29, 2023

andrei-toterman Nov 29, 2023

georgeliao Nov 29, 2023 •

edited

Loading

georgeliao Nov 29, 2023 •

edited

Loading

andrei-toterman Nov 29, 2023

ricab Nov 29, 2023

georgeliao Nov 29, 2023

ricab Nov 29, 2023

ricab Nov 29, 2023

andrei-toterman Nov 29, 2023

georgeliao Nov 30, 2023 •

edited

Loading

andrei-toterman Nov 30, 2023

ricab Nov 30, 2023

georgeliao Nov 29, 2023 •

edited

Loading

andrei-toterman Nov 29, 2023

georgeliao Nov 29, 2023

georgeliao commented Nov 29, 2023

georgeliao commented Nov 30, 2023

townsend2010 commented Nov 30, 2023

townsend2010 commented Nov 30, 2023

andrei-toterman commented Nov 30, 2023

georgeliao Dec 4, 2023

georgeliao commented Dec 5, 2023

townsend2010 left a comment

Replace Qt with std in critical parts of sftp_server #3270

Replace Qt with std in critical parts of sftp_server #3270

Conversation

andrei-toterman commented Oct 24, 2023

codecov bot commented Oct 24, 2023 • edited Loading

Codecov Report

townsend2010 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

georgeliao Nov 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

georgeliao Nov 29, 2023 • edited Loading

Choose a reason for hiding this comment

georgeliao Nov 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

georgeliao Nov 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

georgeliao Nov 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

georgeliao commented Nov 29, 2023

georgeliao commented Nov 30, 2023

townsend2010 commented Nov 30, 2023

townsend2010 commented Nov 30, 2023

andrei-toterman commented Nov 30, 2023

Choose a reason for hiding this comment

georgeliao commented Dec 5, 2023

townsend2010 left a comment

Choose a reason for hiding this comment

codecov bot commented Oct 24, 2023 •

edited

Loading

georgeliao Nov 29, 2023 •

edited

Loading

georgeliao Nov 29, 2023 •

edited

Loading

georgeliao Nov 29, 2023 •

edited

Loading

georgeliao Nov 30, 2023 •

edited

Loading

georgeliao Nov 29, 2023 •

edited

Loading