On 09/28/2017 03:35 PM, Emmanuel Kasper wrote:
> 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 ?
>
That would only happen if you create a VM without CD ROM drive
through CLI or remove it and then re-add it, and even in those
both cases the user always could choose to add it to any other
bus ID as this is just the proposed ID.
> 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
>
>
ide2 is not a special IDE. AFAIS it was just chosen for CD ROM
to have the lower range free for hard disk, which made sense a
few years back where IDE was the default for all VMs.
But yes, as it was my idea and it has hardly any benefit besides
from a little code cleanup and not much to do with the intent of
your series I will take it on my todo list and sent it once a few
of those cleanups are collected :)
cheers,
Thomas
_______________________________________________
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
