-
Notifications
You must be signed in to change notification settings - Fork 164
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
Explicitly prevent mounting over an existing mount #348
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -251,14 +251,6 @@ impl CliArgs { | |
fn main() -> anyhow::Result<()> { | ||
let args = CliArgs::parse(); | ||
|
||
// validate mount point | ||
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. Are we okay moving this validation to the background process? 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. We should be -- it was the only validation we were doing in the foreground process previously, and we shouldn't be doing any work at all before the fork. 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. Makes sense, but we need to update the tests. |
||
if !args.mount_point.exists() || !args.mount_point.is_dir() { | ||
return Err(anyhow!( | ||
"Mount point {} does not exist or it is not a directory", | ||
args.mount_point.display() | ||
)); | ||
} | ||
|
||
if args.foreground { | ||
init_tracing_subscriber(args.foreground, args.log_directory.as_deref()) | ||
.context("failed to initialize logging")?; | ||
|
@@ -370,6 +362,8 @@ fn main() -> anyhow::Result<()> { | |
fn mount(args: CliArgs) -> anyhow::Result<FuseSession> { | ||
const DEFAULT_TARGET_THROUGHPUT: f64 = 10.0; | ||
|
||
validate_mount_point(&args.mount_point)?; | ||
|
||
let addressing_style = args.addressing_style(); | ||
let endpoint = args | ||
.endpoint_url | ||
|
@@ -571,6 +565,39 @@ fn get_maximum_network_throughput(ec2_instance_type: &str) -> anyhow::Result<f64 | |
.ok_or_else(|| anyhow!("no throughput configuration for EC2 instance type {ec2_instance_type}")) | ||
} | ||
|
||
fn validate_mount_point(path: impl AsRef<Path>) -> anyhow::Result<()> { | ||
let mount_point = path.as_ref(); | ||
|
||
if !mount_point.exists() { | ||
return Err(anyhow!("mount point {} does not exist", mount_point.display())); | ||
} | ||
|
||
if !mount_point.is_dir() { | ||
return Err(anyhow!("mount point {} is not a directory", mount_point.display())); | ||
} | ||
|
||
#[cfg(target_os = "linux")] | ||
{ | ||
use procfs::process::Process; | ||
|
||
// This is a best-effort validation, so don't fail if we can't read /proc/self/mountinfo for | ||
// some reason. | ||
let mounts = match Process::myself().and_then(|me| me.mountinfo()) { | ||
Ok(mounts) => mounts, | ||
Err(e) => { | ||
tracing::debug!("failed to read mountinfo, not checking for existing mounts: {e:?}"); | ||
return Ok(()); | ||
} | ||
}; | ||
|
||
if mounts.iter().any(|mount| mount.mount_point == path.as_ref()) { | ||
return Err(anyhow!("mount point {} is already mounted", path.as_ref().display())); | ||
} | ||
} | ||
|
||
Ok(()) | ||
} | ||
|
||
#[cfg(test)] | ||
mod tests { | ||
use super::*; | ||
|
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.
Isn't this platform-specific?