* [pve-devel] [PATCH v2 manager] ui: vm network: allow to override MTU for virtio devices
@ 2022-02-10 11:28 Oguz Bektas
2022-02-10 13:53 ` Aaron Lauterer
0 siblings, 1 reply; 5+ messages in thread
From: Oguz Bektas @ 2022-02-10 11:28 UTC (permalink / raw)
To: pve-devel
we already have the 'mtu' option inside the API, so we can just expose
that option inside the 'Advanced' menu for virtio network interfaces.
Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
---
v1->v2:
* add viewmodel formula 'isVirtio', we use it to set the MTU field
enabled only for virtio interfaces
www/manager6/qemu/NetworkEdit.js | 25 +++++++++++++++++++++++++
1 file changed, 25 insertions(+)
diff --git a/www/manager6/qemu/NetworkEdit.js b/www/manager6/qemu/NetworkEdit.js
index b39cffdc..1e34ad1c 100644
--- a/www/manager6/qemu/NetworkEdit.js
+++ b/www/manager6/qemu/NetworkEdit.js
@@ -19,6 +19,7 @@ Ext.define('PVE.qemu.NetworkInputPanel', {
me.network.macaddr = values.macaddr;
me.network.disconnect = values.disconnect;
me.network.queues = values.queues;
+ me.network.mtu = values.mtu;
if (values.rate) {
me.network.rate = values.rate;
@@ -33,6 +34,16 @@ Ext.define('PVE.qemu.NetworkInputPanel', {
return params;
},
+ viewModel: {
+ data: {
+ network_model: '',
+ },
+ formulas: {
+ isVirtio: (get) => get('network_model') === 'virtio',
+ },
+ },
+
+
setNetwork: function(confid, data) {
var me = this;
@@ -55,6 +66,7 @@ Ext.define('PVE.qemu.NetworkInputPanel', {
me.bridgesel.setNodename(nodename);
},
+
initComponent: function() {
var me = this;
@@ -112,6 +124,7 @@ Ext.define('PVE.qemu.NetworkInputPanel', {
'macaddr',
'rate',
'queues',
+ 'mtu',
];
fields.forEach(function(fieldname) {
me.down('field[name='+fieldname+']').setDisabled(value);
@@ -125,11 +138,13 @@ Ext.define('PVE.qemu.NetworkInputPanel', {
});
}
+
me.column2.push(
{
xtype: 'pveNetworkCardSelector',
name: 'model',
fieldLabel: gettext('Model'),
+ bind: '{network_model}',
value: PVE.qemu.OSDefaults.generic.networkCard,
allowBlank: false,
},
@@ -161,6 +176,16 @@ Ext.define('PVE.qemu.NetworkInputPanel', {
value: '',
allowBlank: true,
},
+ {
+ xtype: 'proxmoxintegerfield',
+ name: 'mtu',
+ fieldLabel: 'MTU',
+ value: '',
+ bind: {
+ disabled: '{!isVirtio}',
+ },
+ allowBlank: true,
+ },
];
me.callParent();
--
2.30.2
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [pve-devel] [PATCH v2 manager] ui: vm network: allow to override MTU for virtio devices
2022-02-10 11:28 [pve-devel] [PATCH v2 manager] ui: vm network: allow to override MTU for virtio devices Oguz Bektas
@ 2022-02-10 13:53 ` Aaron Lauterer
2022-02-10 13:55 ` Aaron Lauterer
2022-02-10 14:08 ` Oguz Bektas
0 siblings, 2 replies; 5+ messages in thread
From: Aaron Lauterer @ 2022-02-10 13:53 UTC (permalink / raw)
To: Proxmox VE development discussion, Oguz Bektas
Looks good AFAICT and does what we want.
We could improve it further by adding a small validator. Otherwise the user will only notice their error once they get the error msg from the API.
A quick way that I came up with that could potentially be done better:
diff --git a/www/manager6/qemu/NetworkEdit.js b/www/manager6/qemu/NetworkEdit.js
index 1e34ad1c..2566d1a8 100644
--- a/www/manager6/qemu/NetworkEdit.js
+++ b/www/manager6/qemu/NetworkEdit.js
@@ -184,6 +184,13 @@ Ext.define('PVE.qemu.NetworkInputPanel', {
bind: {
disabled: '{!isVirtio}',
},
+ validator: function(value) {
+ if (value === "" || (value > 0 && value <= 65520)) {
+ return true;
+ } else {
+ return gettext("must be between 0 and 65520");
+ }
+ },
allowBlank: true,
},
];
Besides this last improvement:
Reviewed-By: Aaron Lauterer <a.lauterer@proxmox.com>
Tested-By: Aaron Lauterer <a.lauterer@proxmox.com>
On 2/10/22 12:28, Oguz Bektas wrote:
> we already have the 'mtu' option inside the API, so we can just expose
> that option inside the 'Advanced' menu for virtio network interfaces.
>
> Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
> ---
> v1->v2:
> * add viewmodel formula 'isVirtio', we use it to set the MTU field
> enabled only for virtio interfaces
>
> www/manager6/qemu/NetworkEdit.js | 25 +++++++++++++++++++++++++
> 1 file changed, 25 insertions(+)
>
> diff --git a/www/manager6/qemu/NetworkEdit.js b/www/manager6/qemu/NetworkEdit.js
> index b39cffdc..1e34ad1c 100644
> --- a/www/manager6/qemu/NetworkEdit.js
> +++ b/www/manager6/qemu/NetworkEdit.js
> @@ -19,6 +19,7 @@ Ext.define('PVE.qemu.NetworkInputPanel', {
> me.network.macaddr = values.macaddr;
> me.network.disconnect = values.disconnect;
> me.network.queues = values.queues;
> + me.network.mtu = values.mtu;
>
> if (values.rate) {
> me.network.rate = values.rate;
> @@ -33,6 +34,16 @@ Ext.define('PVE.qemu.NetworkInputPanel', {
> return params;
> },
>
> + viewModel: {
> + data: {
> + network_model: '',
> + },
> + formulas: {
> + isVirtio: (get) => get('network_model') === 'virtio',
> + },
> + },
> +
> +
> setNetwork: function(confid, data) {
> var me = this;
>
> @@ -55,6 +66,7 @@ Ext.define('PVE.qemu.NetworkInputPanel', {
> me.bridgesel.setNodename(nodename);
> },
>
> +
> initComponent: function() {
> var me = this;
>
> @@ -112,6 +124,7 @@ Ext.define('PVE.qemu.NetworkInputPanel', {
> 'macaddr',
> 'rate',
> 'queues',
> + 'mtu',
> ];
> fields.forEach(function(fieldname) {
> me.down('field[name='+fieldname+']').setDisabled(value);
> @@ -125,11 +138,13 @@ Ext.define('PVE.qemu.NetworkInputPanel', {
> });
> }
>
> +
> me.column2.push(
> {
> xtype: 'pveNetworkCardSelector',
> name: 'model',
> fieldLabel: gettext('Model'),
> + bind: '{network_model}',
> value: PVE.qemu.OSDefaults.generic.networkCard,
> allowBlank: false,
> },
> @@ -161,6 +176,16 @@ Ext.define('PVE.qemu.NetworkInputPanel', {
> value: '',
> allowBlank: true,
> },
> + {
> + xtype: 'proxmoxintegerfield',
> + name: 'mtu',
> + fieldLabel: 'MTU',
> + value: '',
> + bind: {
> + disabled: '{!isVirtio}',
> + },
> + allowBlank: true,
> + },
> ];
>
> me.callParent();
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [pve-devel] [PATCH v2 manager] ui: vm network: allow to override MTU for virtio devices
2022-02-10 13:53 ` Aaron Lauterer
@ 2022-02-10 13:55 ` Aaron Lauterer
2022-02-10 14:08 ` Oguz Bektas
1 sibling, 0 replies; 5+ messages in thread
From: Aaron Lauterer @ 2022-02-10 13:55 UTC (permalink / raw)
To: pve-devel
On 2/10/22 14:53, Aaron Lauterer wrote:
> Looks good AFAICT and does what we want.
>
> We could improve it further by adding a small validator. Otherwise the user will only notice their error once they get the error msg from the API.
>
> A quick way that I came up with that could potentially be done better:
>
> diff --git a/www/manager6/qemu/NetworkEdit.js b/www/manager6/qemu/NetworkEdit.js
> index 1e34ad1c..2566d1a8 100644
> --- a/www/manager6/qemu/NetworkEdit.js
> +++ b/www/manager6/qemu/NetworkEdit.js
> @@ -184,6 +184,13 @@ Ext.define('PVE.qemu.NetworkInputPanel', {
> bind: {
> disabled: '{!isVirtio}',
> },
> + validator: function(value) {
> + if (value === "" || (value > 0 && value <= 65520)) {
> + return true;
> + } else {
> + return gettext("must be between 0 and 65520");
The message here should of course read 1 instead of 0...
> + }
> + },
> allowBlank: true,
> },
> ];
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [pve-devel] [PATCH v2 manager] ui: vm network: allow to override MTU for virtio devices
2022-02-10 13:53 ` Aaron Lauterer
2022-02-10 13:55 ` Aaron Lauterer
@ 2022-02-10 14:08 ` Oguz Bektas
2022-02-10 14:14 ` Aaron Lauterer
1 sibling, 1 reply; 5+ messages in thread
From: Oguz Bektas @ 2022-02-10 14:08 UTC (permalink / raw)
To: Aaron Lauterer; +Cc: Proxmox VE development discussion
On Thu, Feb 10, 2022 at 02:53:19PM +0100, Aaron Lauterer wrote:
> Looks good AFAICT and does what we want.
>
> We could improve it further by adding a small validator. Otherwise the user will only notice their error once they get the error msg from the API.
>
> A quick way that I came up with that could potentially be done better:
>
> diff --git a/www/manager6/qemu/NetworkEdit.js b/www/manager6/qemu/NetworkEdit.js
> index 1e34ad1c..2566d1a8 100644
> --- a/www/manager6/qemu/NetworkEdit.js
> +++ b/www/manager6/qemu/NetworkEdit.js
> @@ -184,6 +184,13 @@ Ext.define('PVE.qemu.NetworkInputPanel', {
> bind: {
> disabled: '{!isVirtio}',
> },
> + validator: function(value) {
> + if (value === "" || (value > 0 && value <= 65520)) {
> + return true;
> + } else {
> + return gettext("must be between 0 and 65520");
> + }
> + },
> allowBlank: true,
> },
> ];
>
>
> Besides this last improvement:
>
> Reviewed-By: Aaron Lauterer <a.lauterer@proxmox.com>
> Tested-By: Aaron Lauterer <a.lauterer@proxmox.com>
>
thanks for the test and review!
instead of a validator function i guess we could just set minValue and
maxValue as well? (since allowBlank is set to true)
seemed to work fine in my short testing just now :)
diff --git a/www/manager6/qemu/NetworkEdit.js b/www/manager6/qemu/NetworkEdit.js
index 1e34ad1c..a3fa5724 100644
--- a/www/manager6/qemu/NetworkEdit.js
+++ b/www/manager6/qemu/NetworkEdit.js
@@ -184,6 +184,8 @@ Ext.define('PVE.qemu.NetworkInputPanel', {
bind: {
disabled: '{!isVirtio}',
},
+ minValue: 1,
+ maxValue: 65520,
allowBlank: true,
},
];
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [pve-devel] [PATCH v2 manager] ui: vm network: allow to override MTU for virtio devices
2022-02-10 14:08 ` Oguz Bektas
@ 2022-02-10 14:14 ` Aaron Lauterer
0 siblings, 0 replies; 5+ messages in thread
From: Aaron Lauterer @ 2022-02-10 14:14 UTC (permalink / raw)
To: Oguz Bektas, Proxmox VE development discussion
On 2/10/22 15:08, Oguz Bektas wrote:
> thanks for the test and review!
>
> instead of a validator function i guess we could just set minValue and
> maxValue as well? (since allowBlank is set to true)
>
> seemed to work fine in my short testing just now :)
>
> diff --git a/www/manager6/qemu/NetworkEdit.js b/www/manager6/qemu/NetworkEdit.js
> index 1e34ad1c..a3fa5724 100644
> --- a/www/manager6/qemu/NetworkEdit.js
> +++ b/www/manager6/qemu/NetworkEdit.js
> @@ -184,6 +184,8 @@ Ext.define('PVE.qemu.NetworkInputPanel', {
> bind: {
> disabled: '{!isVirtio}',
> },
> + minValue: 1,
> + maxValue: 65520,
> allowBlank: true,
> },
> ];
Duh of course...
That does what we want and is much nicer :)
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-02-10 14:15 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-10 11:28 [pve-devel] [PATCH v2 manager] ui: vm network: allow to override MTU for virtio devices Oguz Bektas
2022-02-10 13:53 ` Aaron Lauterer
2022-02-10 13:55 ` Aaron Lauterer
2022-02-10 14:08 ` Oguz Bektas
2022-02-10 14:14 ` Aaron Lauterer
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox