[pve-devel] [PATCH manager 4/4] fix #973: disable iothread for non-virtio drives

[Dominik Csapak]

> comment inline:
>
>> Signed-off-by: Dominik Csapak <d.csapak at proxmox.com>
>> ---
>>   www/manager6/form/ControllerSelector.js |  2 ++
>>   www/manager6/qemu/HDEdit.js             | 14 ++++++++++++++
>>   2 files changed, 16 insertions(+)
>>
>> diff --git a/www/manager6/form/ControllerSelector.js
>> b/www/manager6/form/ControllerSelector.js
>> index 72c4767..f323963 100644
>> --- a/www/manager6/form/ControllerSelector.js
>> +++ b/www/manager6/form/ControllerSelector.js
>> @@ -67,6 +67,7 @@ Ext.define('PVE.form.ControllerSelector', {
>>   		{
>>   		    xtype: 'PVE.form.BusTypeSelector',
>>   		    name: 'controller',
>> +		    itemId: 'controllerbox',
> What for? You do not use it anywhere inside this file (see further
> comments below).
i used it in HDEdit.js
>
>>   		    value: 'ide',
>>   		    noVirtIO: me.noVirtIO,
>>   		    noScsi: me.noScsi,
>> @@ -79,6 +80,7 @@ Ext.define('PVE.form.ControllerSelector', {
>>   			    var field = me.down('field[name=deviceid]');
>>   			    field.setMaxValue(PVE.form.ControllerSelector.maxIds[value]);
>>   			    field.validate();
>> +			    me.fireEvent('change');
> I thought 'Ext.form.FieldContainer' handles this?
what do you mean? the field container does not have a change event by 
default,
so in the subcomponent we fire it for "me" which is the whole fieldcontainer
>
>>   			}
>>   		    }
>>   		},
>> diff --git a/www/manager6/qemu/HDEdit.js b/www/manager6/qemu/HDEdit.js
>> index a360f23..5a0f851 100644
>> --- a/www/manager6/qemu/HDEdit.js
>> +++ b/www/manager6/qemu/HDEdit.js
>> @@ -105,6 +105,16 @@ Ext.define('PVE.qemu.HDInputPanel', {
>>   	me.hdfilesel.setStorage(undefined, nodename);
>>       },
>>   
>> +    controllerChange: function() {
>> +	var me = this;
>> +	var iothreadbox = me.down('#iothreadbox');
>> +	var controllerbox = me.down('#controllerbox');
> not nice...
>
>> +	if (iothreadbox && controllerbox) {
>> +	    // disable iothread box when controller != virtio
>> +
>>      iothreadbox.setDisabled(!me.down('#controllerbox').value.match(/^virtio/));
>> +	}
>> +    },
> what about virtio-scsi (as mentioned by alexandre)?
yes i missed this, but currently we dont save the iothread checkbox when 
the drive != virtio
for this i have to rewrite the patch (since this is non trivial, what if 
someone has scsi but not
the virtio controller? what if he changes that after setting iothread?)
please wait for a v2
>
>>       initComponent : function() {
>>   	var me = this;
>>   
>> @@ -118,6 +128,8 @@ Ext.define('PVE.qemu.HDInputPanel', {
>>   		vmconfig: me.insideWizard ? {ide2: 'cdrom'} : {}
>>   	    });
>>   	    me.column1.push(me.bussel);
>> +	    me.mon(me.bussel, 'change', me.controllerChange, me);
>> +	    me.mon(me.bussel, 'afterrender', me.controllerChange, me);
>>   	}
>>   
>>   	if (me.unused) {
>> @@ -240,7 +252,9 @@ Ext.define('PVE.qemu.HDInputPanel', {
>>   	});
>>   
>>   	me.column2.push({
>> +	    itemId: 'iothreadbox',
>>   	    xtype: 'pvecheckbox',
>> +	    disabled: me.insideWizard || (me.confid && !me.confid.match(/^virtio/)),
>>   	    fieldLabel: gettext('IO thread'),
>>   	    name: 'iothread'
> Why you you introduce new itemIds here? You can simply
> find a field using:
>
> me.down('field[name=iothread]')
>
> I also think we should use view controllers when possible, and thus
> use 'reference' instead of itemIDs?
i try to do it better in my v2 ;)