Re: [PATCH proxmox v2 3/3] pbs-api-types: zfs: define schema for recordsize

["Nicolas Frey" <n.frey@proxmox.com>]

thanks for the v2 and adding a test! one small style nit inline

On Wed Aug 12, 2026 at 1:03 PM CEST, Shan Shaji wrote:
> add schema and verifcation function for recordsize to check against
> allowed zfs constraints. Without the verification function the dataset
> will be created using the default 128k recordsize.
>
> Signed-off-by: Shan Shaji <s.shaji@proxmox.com>
> ---
>
>  changes since v1: Please see the cover letter.
>
>  pbs-api-types/src/zfs.rs | 68 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 68 insertions(+)
>
> diff --git a/pbs-api-types/src/zfs.rs b/pbs-api-types/src/zfs.rs
> index 2cfbd403..dea84b2e 100644
> --- a/pbs-api-types/src/zfs.rs
> +++ b/pbs-api-types/src/zfs.rs
> @@ -1,3 +1,4 @@
> +use anyhow::{Error, bail};
>  use serde::{Deserialize, Serialize};
>
>  #[cfg(feature = "enum-fallback")]
> @@ -6,6 +7,7 @@ use proxmox_schema::*;
>
>  const_regex! {
>      pub ZPOOL_NAME_REGEX = r"^[a-zA-Z][a-z0-9A-Z\-_.:]+$";
> +    ZFS_RECORD_SIZE_REGEX = r"^(?i)([1-9][0-9]*)([km])?$";
>  }
>
>  pub const ZFS_ASHIFT_SCHEMA: Schema = IntegerSchema::new("Pool sector size exponent.")
> @@ -18,6 +20,39 @@ pub const ZPOOL_NAME_SCHEMA: Schema = StringSchema::new("ZFS Pool Name")
>      .format(&ApiStringFormat::Pattern(&ZPOOL_NAME_REGEX))
>      .schema();
>
> +fn verify_zfs_recordsize(value: &str) -> Result<(), Error> {
> +    if let Some(caps) = ZFS_RECORD_SIZE_REGEX.captures(value) {

nit: you could instead use `let Some(x)` so the whole block does not need to be nested:

let Some(caps) = ZFS_RECORD_SIZE_REGEX.captures(value) else {
	bail!("Invalid format. Use numbers with optional k or m suffix (e.g., 16k).");
};

.. rest of the code

though no need to send a v3 for it, as it's just a style nit

> +        let range_error = "Value must be a power of 2 between 512 and 16m";
> +
> +        let Ok(number) = caps[1].parse::<u32>() else {
> +            bail!(range_error);
> +        };
> +
> +        let multiplier = match caps.get(2).map(|v| v.as_str()) {
> +            Some(suffix) if suffix.eq_ignore_ascii_case("k") => 1024,
> +            Some(suffix) if suffix.eq_ignore_ascii_case("m") => 1024 * 1024,
> +            _ => 1,
> +        };
> +
> +        let bytes = number
> +            .checked_mul(multiplier)
> +            .filter(|&b| b.is_power_of_two() && (512..=16 * 1024 * 1024).contains(&b));
> +
> +        if bytes.is_none() {
> +            bail!(range_error);
> +        }
> +
> +        Ok(())
> +    } else {
> +        bail!("Invalid format. Use numbers with optional k or m suffix (e.g., 16k).")
> +    }
> +}
> +
> +pub const ZFS_RECORD_SIZE_SCHEMA: Schema =
> +    StringSchema::new("ZFS Recordsize. Use numbers with optional k or m suffix (e.g., 128k).")
> +        .format(&ApiStringFormat::VerifyFn(verify_zfs_recordsize))
> +        .schema();
> +
>  #[api(default: "On")]
>  #[derive(Debug, Copy, Clone, PartialEq, Eq, Serialize, Deserialize)]
>  #[serde(rename_all = "lowercase")]
> @@ -84,3 +119,36 @@ pub struct ZpoolListItem {
>      /// ZFS deduplication ratio
>      pub dedup: f64,
>  }
> +
> +#[cfg(test)]
> +mod tests {
> +
> +    use super::verify_zfs_recordsize;
> +
> +    #[test]
> +    fn test_zfs_record_size_verify_fn() {
> +        let format_error = "Invalid format. Use numbers with optional k or m suffix (e.g., 16k).";
> +        let range_error = "Value must be a power of 2 between 512 and 16m";
> +
> +        let valid_inputs = ["512", "1024", "16m", "16384k", "16777216"];
> +        for input in valid_inputs {
> +            assert!(verify_zfs_recordsize(input).is_ok());
> +        }
> +
> +        let range_error_inputs = ["31m", "256", "31m", "32m", "4294967297"];
> +        for input in range_error_inputs {
> +            assert_eq!(
> +                verify_zfs_recordsize(input).unwrap_err().to_string(),
> +                range_error
> +            );
> +        }
> +
> +        let format_error_inputs = ["0", "k12", "16g", "1.5m", "-512", "0512"];
> +        for input in format_error_inputs {
> +            assert_eq!(
> +                verify_zfs_recordsize(input).unwrap_err().to_string(),
> +                format_error
> +            );
> +        }
> +    }
> +}

thanks for adding a test too!