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 1125F1FF13A for ; Wed, 13 May 2026 16:05:50 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 049E9106FE; Wed, 13 May 2026 16:05:48 +0200 (CEST) Message-ID: <67e7df21-8881-4b91-8b07-d8af3bb48a4b@proxmox.com> Date: Wed, 13 May 2026 16:05:09 +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: David Riley , pve-devel@lists.proxmox.com References: <20260512134852.142044-1-d.riley@proxmox.com> <20260512134852.142044-2-d.riley@proxmox.com> Content-Language: en-US From: Fiona Ebner In-Reply-To: <20260512134852.142044-2-d.riley@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1778681106235 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.009 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. [proxmox.com,openzfs.github.io] Message-ID-Hash: WKEHAMLJPSBJC62EOBD4N3ILWELGOYD7 X-Message-ID-Hash: WKEHAMLJPSBJC62EOBD4N3ILWELGOYD7 X-MailFrom: f.ebner@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: 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 > + 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, > }, > ], > });