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

[David Riley <d.riley@proxmox.com>]

Thanks for the review.
I will send a v2 incorporating your feedback.

On 5/13/26 4:05 PM, Fiona Ebner wrote:
> 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
ack. Will fix.
>
>> +            if (!value || value === '') {
> Nit: the second half is redundant, because !'' is already true
ack.
>
>> +                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();
ack.
>
>> +
>> +            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.
Agreed, I'll combine these into a single check for the v2.
>
>> +                );
>> +            }
>> +
>> +            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,
>>           },
>>       ],
>>   });