From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id 4A0071FF14C for ; Fri, 15 May 2026 08:58:11 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 80EE5D35F; Fri, 15 May 2026 08:58:09 +0200 (CEST) Message-ID: <6101f11a-0c61-44de-86b2-dafa0eb540c2@proxmox.com> Date: Fri, 15 May 2026 08:57:26 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH pve-manager 1/2] fix #3936: ui: storage: add validation for ZFS blocksize To: Fiona Ebner , pve-devel@lists.proxmox.com References: <20260512134852.142044-1-d.riley@proxmox.com> <20260512134852.142044-2-d.riley@proxmox.com> <67e7df21-8881-4b91-8b07-d8af3bb48a4b@proxmox.com> Content-Language: en-US From: David Riley In-Reply-To: <67e7df21-8881-4b91-8b07-d8af3bb48a4b@proxmox.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1778828240849 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.303 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [openzfs.github.io] Message-ID-Hash: BWY6LD2UO6XVHWVICSNPLX2V767ZHGTL X-Message-ID-Hash: BWY6LD2UO6XVHWVICSNPLX2V767ZHGTL X-MailFrom: d.riley@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: 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 > Some nits, but otherwise: > > Reviewed-by: Fiona Ebner > >> --- >> 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 ' ' + 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, >> }, >> ], >> });