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
gadget: improve device lookup, add helper for mount point lookup #6873
gadget: improve device lookup, add helper for mount point lookup #6873
Conversation
Look out for symlinks that point nowhere. The osutil.FileExists() helper already uses os.Stat() internally and that will catch symlinks pointing nowhere. However, add a safety check in case anyone tweaks the osutil implementation. Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
Add a helper for locating mount points of given filesystem structure. Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
It is in Go 1.12+ only. Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
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.
Looks good, I've added one comment about a case where mount entries shadow older entries. I also wonder if the kind of lookup should not belong in osutil
(with a helper function to match so that PositionedVolume
can stay here), simply because of the fact that looking at mount table is tricky.
gadget/device_linux.go
Outdated
return "", fmt.Errorf("cannot read mount info: %v", err) | ||
} | ||
for _, entry := range mountInfo { | ||
if entry.MountSource != devpath || entry.FsType != ps.Filesystem { |
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.
I would add a small snippet like this here:
if mountPoint != "" && (mountPoint == entry.MountDir || strings.HasPrefix(mountPoint, entry.MountDir + "/")) {
// mount entry clobbers visibility to previously discovered match
mountPoint = "" // clobbered
}
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.
There's a break
couple of lines down, so the loop will finish once a match is found. This makes me think, I should restructure the loop bit to make the lookup easier to read.
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.
Pushed an update, should be clearer now.
Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
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.
+1
Add a helper for a filesystem structure mount point lookup. Take a live core18 system, for a structure
EFI System
defined like this:we should be able to idenfity
/boot/efi
as its mount point.Improve the lookup of matching devices. Although
os.FileExists()
already doesos.Stat()
and that will catch symlinks pointing nowhere, try the belt and suspenders approach to be on the safe side should somebody tweak the implementation.cc @cmatsuoka