On 09/27/2017 10:47 AM, Thomas Lamprecht wrote: > On 09/26/2017 02:17 PM, Emmanuel Kasper wrote: >> The bus selector is displayed when we add a Hard Disk or CD Drive. >> When it is displayed, we *always* preselect the next available >> slot on the controller of our choice. >> So this test is not needed. >> >> We keep the test on the string value of 'autoselect' to select >> a bus position when adding a CD Drive. > > Looks good, at least with git show -w :) > Reviewed-by: Thomas Lamprecht <t.lampre...@proxmox.com> > > A side comment still inline below. > >> --- >> www/manager6/form/ControllerSelector.js | 53 >> ++++++++++++++++----------------- >> www/manager6/qemu/HDEdit.js | 2 +- >> 2 files changed, 27 insertions(+), 28 deletions(-) >> >> diff --git a/www/manager6/form/ControllerSelector.js >> b/www/manager6/form/ControllerSelector.js >> index 002134ef..045e7d0d 100644 >> --- a/www/manager6/form/ControllerSelector.js >> +++ b/www/manager6/form/ControllerSelector.js >> @@ -58,37 +58,36 @@ Ext.define('PVE.form.ControllerSelector', { >> var me = this; >> me.vmconfig = Ext.apply({}, vmconfig); >> - if (autoSelect) { >> - var clist = ['ide', 'virtio', 'scsi', 'sata']; >> - if (autoSelect === 'cdrom') { >> - clist = ['ide', 'scsi', 'sata']; >> - if (!Ext.isDefined(me.vmconfig.ide2)) { >> - me.down('field[name=controller]').setValue('ide'); >> - me.down('field[name=deviceid]').setValue(2); >> - return; >> - } >> - } else { >> - // in most cases we want to add a disk to the same controller >> - // we previously used >> - clist = me.sortByPreviousUsage(me.vmconfig, clist); >> + >> + var clist = ['ide', 'virtio', 'scsi', 'sata']; >> + if (autoSelect === 'cdrom') { >> + clist = ['ide', 'scsi', 'sata']; >> + if (!Ext.isDefined(me.vmconfig.ide2)) { > > AFAIS, this is just the fastpath? > As performance is rarely a problem when iterating a very small array, > maybe just remove this all but the line where clist gets re-set > and let the general-purpose code below handle this case too?
but this would mean that a newly added CD ROM drive is not anymore added to the index 2 on the IDE bus by default ? I am not sure of the implications of these for the machines type we emulate, so I would prefer not to change this in this patch serie _______________________________________________ pve-devel mailing list pve-devel@pve.proxmox.com https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel