-
Notifications
You must be signed in to change notification settings - Fork 650
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix symlink resolution when mounting #3673
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3673 +/- ##
==========================================
- Coverage 88.92% 88.92% -0.01%
==========================================
Files 254 254
Lines 14299 14309 +10
==========================================
+ Hits 12716 12724 +8
- Misses 1583 1585 +2 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you Trevor! Superficial review from my side, but it looks like it's headed in the right direction. I have a couple of comments inline.
src/daemon/daemon.cpp
Outdated
? std::make_unique<SSHFSMountHandler>(vm, config->ssh_key_provider.get(), target, mount) | ||
: vm->make_native_mount_handler(target, mount); | ||
? std::make_unique<SSHFSMountHandler>(vm, config->ssh_key_provider.get(), target, std::move(mount)) | ||
: vm->make_native_mount_handler(target, std::move(mount)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one doesn't help much unless you change make_native_mount_handler
to take by value and move too. Otherwise you're asking for one extra temporary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey Trevor, this works well for what it does and the code looks good. However, currently it covers only paths ending in a symlink. I guess we should cover symlinks anywhere in the path, don't you think?
ricab@ricab-XPS-9320:~/tmp/asdf $ ln -s .. l4
ricab@ricab-XPS-9320:~/tmp/asdf $ cd
ricab@ricab-XPS-9320:~ $ mp mount tmp/asdf/l4/subl-mdpreview/ amazed-topi:l4
ricab@ricab-XPS-9320:~ $ mp info amazed-topi
Name: amazed-topi
State: Running
Snapshots: 2
IPv4: 10.251.95.73
Release: Ubuntu 24.04.1 LTS
Image hash: 78547d336e4c (Ubuntu 24.04 LTS)
CPU(s): 2
Load: 0.00 0.00 0.00
Disk usage: 2.0GiB out of 4.8GiB
Memory usage: 342.3MiB out of 955.7MiB
Mounts: /home/ricab/tmp/asdf/l4/subl-mdpreview => l4
UID map: 1000:default
GID map: 1000:default
I expected the source to become just /home/ricab/tmp/subl-mdpreview
src/utils/vm_mount.cpp
Outdated
|
||
if (err) | ||
throw std::runtime_error( | ||
fmt::format("Mount symlink source path \"{}\" could not be made weakly canonical: {}.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would end up being shown to the user, right? If so, I would try to make it more user friendly. Perhaps:
fmt::format("Mount symlink source path \"{}\" could not be made weakly canonical: {}.", | |
fmt::format("Could not resolve symlinks in mount source path \"{}\": {}.", |
Hey Trevor, a quick experiment I did: #include <filesystem>
#include <iostream>
using namespace std;
namespace fs = std::filesystem;
int main()
{
// l4 -> ..
// l2 -> ../subl-mdpreview
auto src_path = "/home/ricab/tmp/asdf/l4/asdf/l4/asdf/l2/";
error_code ec{};
auto dst_path = fs::weakly_canonical(src_path, ec);
if (!ec)
cout << "\"" << src_path << "\" == " << dst_path << endl;
// prints "/home/ricab/tmp/asdf/l4/asdf/l4/asdf/l2/" == "/home/ricab/tmp/subl-mdpreview"
return bool(ec);
}
|
06347b0
to
298a379
Compare
9b71479
to
ccac3af
Compare
I wonder if we should check elsewhere in the repo for usage of |
fixes #1722
Symbolic links are now resolved when performing a mount.
Previously they were not resolved.