Skip to content

Commit

Permalink
Address PR feedback (squash on merge please)
Browse files Browse the repository at this point in the history
- improved error messages
- minor refactor of value parsing.

Validated that the defaults and overrides are set when mounting the
filesystem.
  • Loading branch information
adpeace committed Nov 17, 2024
1 parent b5bda60 commit 88660b2
Showing 1 changed file with 38 additions and 21 deletions.
59 changes: 38 additions & 21 deletions mountpoint-s3/src/fs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -153,36 +153,53 @@ where
self.next_handle.fetch_add(1, Ordering::SeqCst)
}

Check warning on line 155 in mountpoint-s3/src/fs.rs

View workflow job for this annotation

GitHub Actions / Formatting

Diff in /home/runner/work/mountpoint-s3/mountpoint-s3/mountpoint-s3/src/fs.rs
/// Helper to return the u16 value in an environment variable, or panic. Useful for unstable overrides.
fn parse_env_var_to_u16(var_name: &str, var_value: std::ffi::OsString) -> u16 {
var_value.to_string_lossy().parse::<u16>().unwrap_or_else(|_| panic!(
"Invalid value for environment variable {}. Must be positive integer.",
var_name
))
}

pub async fn init(&self, config: &mut KernelConfig) -> Result<(), libc::c_int> {
const ENV_VAR_KEY_MAX_BACKGROUND: &str = "UNSTABLE_MOUNTPOINT_MAX_BACKGROUND";
const ENV_VAR_KEY_CONGESTION_THRESHOLD: &str = "UNSTABLE_MOUNTPOINT_CONGESTION_THRESHOLD";
let _ = config.add_capabilities(fuser::consts::FUSE_DO_READDIRPLUS);
// Set max_background FUSE parameter to 64 by default, or override with environment variable:
// Set max_background FUSE parameter to 64 by default, or override with environment variable.
// NOTE: Support for this environment variable may be removed in future without notice.

Check warning on line 169 in mountpoint-s3/src/fs.rs

View workflow job for this annotation

GitHub Actions / Formatting

Diff in /home/runner/work/mountpoint-s3/mountpoint-s3/mountpoint-s3/src/fs.rs
if let Some(user_max_background) = std::env::var_os("UNSTABLE_MAX_BACKGROUND") {
let max_background = user_max_background
.to_string_lossy()
.parse::<u16>()
.expect("invalid env var value for UNSTABLE_MAX_BACKGROUND");
let old = config
.set_max_background(max_background)
.expect("unable to set max background");
tracing::warn!("set max background to {} from {}", max_background, old)
if let Some(user_max_background) = std::env::var_os(ENV_VAR_KEY_MAX_BACKGROUND) {
let max_background = Self::parse_env_var_to_u16(ENV_VAR_KEY_MAX_BACKGROUND, user_max_background);
let old = config.set_max_background(max_background).unwrap_or_else(|_| panic!(
"Unable to set FUSE max_background configuration to {}",
max_background
));
tracing::warn!(
"Successfully overridden FUSE max_background configuration to {} (was {}) from unstable environment variable.",
max_background,
old
);
} else {
let _ = config
.set_max_background(64)
.expect("unable to set max background to default value");
.expect("unable to set FUSE max_background to default value");
}
// Override FUSE congestion threshold if environment variable is present:

// Override FUSE congestion threshold if environment variable is present.
// NOTE: Support for this environment variable may be removed in future without notice.
if let Some(user_congestion_threshold) = std::env::var_os("UNSTABLE_CONGESTION_THRESHOLD") {
let congestion_threshold = user_congestion_threshold
.to_string_lossy()
.parse::<u16>()
.expect("invalid env var value for UNSTABLE_CONGESTION_THRESHOLD");
let old = config
.set_congestion_threshold(congestion_threshold)
.expect("unable to set congestion threshold");
tracing::warn!("set congestion threshold to {} from {}", congestion_threshold, old);
if let Some(user_congestion_threshold) = std::env::var_os(ENV_VAR_KEY_CONGESTION_THRESHOLD) {

Check warning on line 189 in mountpoint-s3/src/fs.rs

View workflow job for this annotation

GitHub Actions / Formatting

Diff in /home/runner/work/mountpoint-s3/mountpoint-s3/mountpoint-s3/src/fs.rs
let congestion_threshold =
Self::parse_env_var_to_u16(ENV_VAR_KEY_CONGESTION_THRESHOLD, user_congestion_threshold);
let old = config.set_congestion_threshold(congestion_threshold).unwrap_or_else(|_| panic!(
"unable to set FUSE congestion_threshold to {}",
congestion_threshold
));
tracing::warn!(
"Successfully overridden FUSE congestion_threshold configuration to {} (was {}) from unstable environment variable.",
congestion_threshold, // Note: fixed a bug here, was using max_background
old
);
}

if self.config.allow_overwrite {
// Overwrites require FUSE_ATOMIC_O_TRUNC capability on the host, so we will panic if the
// host doesn't support it.
Expand Down

0 comments on commit 88660b2

Please sign in to comment.