Re: [PATCH pve-manager 1/2] fix #3936: ui: storage: add validation for ZFS blocksize

[Fiona Ebner <f.ebner@proxmox.com>]

Am 12.05.26 um 3:47 PM schrieb David Riley:
> Add validation to the ZFS storage blocksize form field to prevent
> misconfigurations.
> 
> Validation [0][1]:
> * Range: 512 bytes to 16 MiB
> * Format: Allow positive integers with optional 'k' or 'm' suffix.
> * Constraint: Must be a power of two.
> 
> Link: https://bugzilla.proxmox.com/show_bug.cgi?id=3936
> 
> [0] https://openzfs.github.io/openzfs-docs/man/v2.4/7/zfsprops.7.html#volblocksize
> [1] https://openzfs.github.io/openzfs-docs/man/v2.4/7/zfsprops.7.html#recordsize
> 
> Signed-off-by: David Riley <d.riley@proxmox.com>

Some nits, but otherwise:

Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>

> ---
>  www/manager6/Utils.js               | 34 +++++++++++++++++++++++++++++
>  www/manager6/storage/ZFSEdit.js     |  1 +
>  www/manager6/storage/ZFSPoolEdit.js |  1 +
>  3 files changed, 36 insertions(+)
> 
> diff --git a/www/manager6/Utils.js b/www/manager6/Utils.js
> index 01e80682..a8f684a0 100644
> --- a/www/manager6/Utils.js
> +++ b/www/manager6/Utils.js
> @@ -274,6 +274,40 @@ Ext.define('PVE.Utils', {
>              return '<i class="fa fa-' + iconCls + '"></i> ' + value;
>          },
>  
> +        validate_zfs_blocksize: function (value) {

Style nit: new function names should be camelCase

https://pve.proxmox.com/wiki/Javascript_Style_Guide#Casing

> +            if (!value || value === '') {

Nit: the second half is redundant, because !'' is already true

> +                return true;
> +            }
> +
> +            let match = value.match(/^([1-9][0-9]*)([km])?$/i);
> +            if (!match) {
> +                return gettext(
> +                    'Invalid format. Use numbers with optional k or m suffix (e.g., 16k).',
> +                );
> +            }
> +
> +            let bytes = parseInt(match[1], 10);
> +            let suffix = match[2] ? match[2].toLowerCase() : '';

Nit: in this case, it could be:

let suffix = match[2]?.toLowerCase();

> +
> +            if (suffix === 'k') {
> +                bytes *= 1024;
> +            } else if (suffix === 'm') {
> +                bytes *= 1024 * 1024;
> +            }
> +
> +            if (bytes < 512 || (bytes & (bytes - 1)) !== 0) {
> +                return gettext(
> +                    'Value must be a power of 2 and at least 512 (e.g., 4k, 8k, 16k, 32k).',

Not sure if it's worth splitting the check? The text could be "a power
of 2 between 512 and 16m", then you'd implicitly cover the 'm' suffix as
an example too.

> +                );
> +            }
> +
> +            if (bytes > 16 * 1024 * 1024) {
> +                return gettext('Value is too large (max 16 MiB).');
> +            }
> +
> +            return true;
> +        },
> +
>          render_pbs_fingerprint: (fp) => fp.substring(0, 23),
>  
>          render_backup_encryption: function (v, meta, record) {
> diff --git a/www/manager6/storage/ZFSEdit.js b/www/manager6/storage/ZFSEdit.js
> index e9d24642..ef18fa11 100644
> --- a/www/manager6/storage/ZFSEdit.js
> +++ b/www/manager6/storage/ZFSEdit.js
> @@ -67,6 +67,7 @@ Ext.define('PVE.storage.ZFSInputPanel', {
>                  value: '4k',
>                  fieldLabel: gettext('Block Size'),
>                  allowBlank: false,
> +                validator: PVE.Utils.validate_zfs_blocksize,
>              },
>              {
>                  xtype: me.isCreate ? 'textfield' : 'displayfield',
> diff --git a/www/manager6/storage/ZFSPoolEdit.js b/www/manager6/storage/ZFSPoolEdit.js
> index 0066dfef..2d7b4498 100644
> --- a/www/manager6/storage/ZFSPoolEdit.js
> +++ b/www/manager6/storage/ZFSPoolEdit.js
> @@ -106,6 +106,7 @@ Ext.define('PVE.storage.ZFSPoolInputPanel', {
>              emptyText: '16k',
>              fieldLabel: gettext('Block Size'),
>              allowBlank: true,
> +            validator: PVE.Utils.validate_zfs_blocksize,
>          },
>      ],
>  });