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

["Shan Shaji" <s.shaji@proxmox.com>]

Hi,

Thanks for testing and verifying the code changes.

On Wed Aug 12, 2026 at 3:41 PM CEST, Nicolas Frey wrote:
> 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).");
> };

ahh, right! this is much more cleaner. I did used `let..else` inside the block
but didn't change it here. 

> .. 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);
>> +        };
            ^^^^ here
>> +        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!