[pve-devel] [PATCH manager v3 1/8] Do not use 'autoselect' as a boolean when preselecting a bus

[Thomas Lamprecht]

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.lamprecht at 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