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 085791FF15E for ; Fri, 18 Oct 2024 09:42:07 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 704EE1CA14; Fri, 18 Oct 2024 09:42:34 +0200 (CEST) Message-ID: Date: Fri, 18 Oct 2024 09:42:29 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird From: Lukas Wagner To: Proxmox VE development discussion , Daniel Kral References: <20241016164711.934544-1-d.kral@proxmox.com> <20241016164711.934544-4-d.kral@proxmox.com> Content-Language: de-AT, en-US In-Reply-To: <20241016164711.934544-4-d.kral@proxmox.com> 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 Subject: Re: [pve-devel] [PATCH manager 3/5] fix #5430: ui: vm: allow editing cdrom aio and cache options X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox VE development discussion Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "pve-devel" Some suggestions inline. Skimmed over the code to spot style issues, correctness was not really checked. On 2024-10-16 18:47, Daniel Kral wrote: > Adds cache and async I/O selectors to the CDROM Drive Edit modal window > in the "Hardware" tab. This allows users to set these options in the > WebGUI when the VM fails to start because the underlying storage > (driver) does not support a specific set of configurations. > > Signed-off-by: Daniel Kral > --- > www/manager6/qemu/CDEdit.js | 30 ++++++++++++++++++++++++++++-- > 1 file changed, 28 insertions(+), 2 deletions(-) > > diff --git a/www/manager6/qemu/CDEdit.js b/www/manager6/qemu/CDEdit.js > index 3cc16205..f15905e7 100644 > --- a/www/manager6/qemu/CDEdit.js > +++ b/www/manager6/qemu/CDEdit.js > @@ -7,6 +7,7 @@ Ext.define('PVE.qemu.CDInputPanel', { > onGetValues: function(values) { > var me = this; > > + var params = {}; Note: For new code we should use `let` or `const` insted of `var` [1]. When touching existing code can IMO also change it to `let`, but of course familiarize yourself first with the differences between the three and make sure that you don't introduce any unwanted changes (e.g. due to changed scoping). [1]: https://pve.proxmox.com/wiki/Javascript_Style_Guide > var confid = me.confid || values.controller + values.deviceid; > > me.drive.media = 'cdrom'; > @@ -18,7 +19,8 @@ Ext.define('PVE.qemu.CDInputPanel', { > me.drive.file = 'none'; > } > > - var params = {}; > + PVE.Utils.propertyStringSet(me.drive, values.cache, 'cache'); > + PVE.Utils.propertyStringSet(me.drive, values.aio, 'aio'); > > params[confid] = PVE.Parser.printQemuDrive(me.drive); > > @@ -46,6 +48,9 @@ Ext.define('PVE.qemu.CDInputPanel', { > values.cdimage = drive.file; > } > > + values.cache = drive.cache || '__default__'; > + values.aio = drive.aio || '__default__'; > + > me.drive = drive; > > me.setValues(values); > @@ -118,6 +123,27 @@ Ext.define('PVE.qemu.CDInputPanel', { > > me.items = items; > > + // those are only useful for specific niche use cases > + if (!me.insideWizard) { > + me.advancedColumn1 = [ > + { > + xtype: 'CacheTypeSelector', > + name: 'cache', > + value: '__default__', > + fieldLabel: gettext('Cache'), > + }, > + ]; > + me.advancedColumn2 = [ > + { > + xtype: 'AsyncIOTypeSelector', > + name: 'aio', > + value: '__default__', > + fieldLabel: gettext('Async IO'), > + allowBlank: false, > + }, > + ]; > + } > + > me.callParent(); > }, > }); > @@ -125,7 +151,7 @@ Ext.define('PVE.qemu.CDInputPanel', { > Ext.define('PVE.qemu.CDEdit', { > extend: 'Proxmox.window.Edit', > > - width: 400, > + width: 550, This change is not mentioned in the commit message and should probably be split out into its own commit. > > initComponent: function() { > var me = this; -- - Lukas _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel