-
Notifications
You must be signed in to change notification settings - Fork 642
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?
Changes from 4 commits
e5e2ec4
4bb0fc1
e352056
607afba
06347b0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,6 +17,8 @@ | |
|
||
#include <multipass/vm_mount.h> | ||
|
||
#include <multipass/file_ops.h> | ||
|
||
#include <QJsonArray> | ||
|
||
namespace mp = multipass; | ||
|
@@ -111,6 +113,36 @@ | |
{ | ||
} | ||
|
||
void mp::VMMount::resolve_source_path() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good call making this a method on the mount. Thinking about it a little further now, I wonder if we should just call this in the constructor and avoid the intermediate state at all. But I haven't looked deep for implications. WDYT? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was thinking about the same thing. This seems to be a private function to me. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think doing it in the constructor would be good, I'll look into it and any implications it has. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems that doing it in the constructor makes it so that the source path is persisted as the place the symlink points to. This changes the behavior a bit so that symlinks are only ever resolved once instead of when the daemon restarts. It's still a fix for the issue but acts a bit different, what do you think? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm. I wouldn't mind just replacing the original path with the target of the symlink, but doing so completely. IOW, making sure that the source path is discarded and has no further effect. I would like to avoid recording something for a while (either persisted or just in memory) and then something else later. Also, keep in mind that this will need to deal with "legacy" mounts, i.e., mounts that people established before this PR. You may find that they would already be transformed when the daemon restarts, but it is something to check. |
||
{ | ||
std::error_code err; | ||
fs::path path{source_path}; | ||
|
||
auto status = MP_FILEOPS.symlink_status(path, err); | ||
|
||
if (status.type() == fs::file_type::symlink) | ||
{ | ||
auto symlink_path = MP_FILEOPS.read_symlink(path, err); | ||
|
||
if (err) | ||
throw std::runtime_error( | ||
fmt::format("Mount symlink source path \"{}\" could not be read: {}.", source_path, err.message())); | ||
|
||
if (symlink_path.is_relative()) | ||
symlink_path = path.parent_path() / symlink_path; | ||
|
||
path = fs::weakly_canonical(symlink_path, err); | ||
|
||
if (err) | ||
throw std::runtime_error( | ||
fmt::format("Mount symlink source path \"{}\" could not be made weakly canonical: {}.", | ||
source_path, | ||
err.message())); | ||
|
||
source_path = path.string(); | ||
} | ||
} | ||
|
||
QJsonObject mp::VMMount::serialize() const | ||
{ | ||
QJsonObject ret; | ||
|
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.