From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <t.lamprecht@proxmox.com>
Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68])
(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)
key-exchange X25519 server-signature RSA-PSS (2048 bits))
(No client certificate requested)
by lists.proxmox.com (Postfix) with ESMTPS id 7EE6871AF4
for <pve-devel@lists.proxmox.com>; Fri, 20 May 2022 09:41:13 +0200 (CEST)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
by firstgate.proxmox.com (Proxmox) with ESMTP id 6A243F98E
for <pve-devel@lists.proxmox.com>; Fri, 20 May 2022 09:41:13 +0200 (CEST)
Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com
[94.136.29.106])
(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)
key-exchange X25519 server-signature RSA-PSS (2048 bits))
(No client certificate requested)
by firstgate.proxmox.com (Proxmox) with ESMTPS id 12284F981
for <pve-devel@lists.proxmox.com>; Fri, 20 May 2022 09:41:12 +0200 (CEST)
Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1])
by proxmox-new.maurer-it.com (Proxmox) with ESMTP id D46CC42D06
for <pve-devel@lists.proxmox.com>; Fri, 20 May 2022 09:41:11 +0200 (CEST)
Message-ID: <c175eac4-b9c9-dbe9-2a93-bdb4d65ab903@proxmox.com>
Date: Fri, 20 May 2022 09:41:10 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:101.0) Gecko/20100101
Thunderbird/101.0
Content-Language: en-GB
To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>,
Oguz Bektas <o.bektas@proxmox.com>
References: <20220519084816.193338-1-o.bektas@proxmox.com>
From: Thomas Lamprecht <t.lamprecht@proxmox.com>
In-Reply-To: <20220519084816.193338-1-o.bektas@proxmox.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
X-SPAM-LEVEL: Spam detection results: 0
AWL 0.376 Adjusted score from AWL reputation of From: address
BAYES_00 -1.9 Bayes spam probability is 0 to 1%
KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment
NICE_REPLY_A -0.717 Looks like a legit reply (A)
SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record
SPF_PASS -0.001 SPF: sender matches SPF record
T_SCC_BODY_TEXT_LINE -0.01 -
Subject: Re: [pve-devel] [PATCH v5 manager] ui: vm network: allow to
override MTU for virtio devices
X-BeenThere: pve-devel@lists.proxmox.com
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Proxmox VE development discussion <pve-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pve-devel/>
List-Post: <mailto:pve-devel@lists.proxmox.com>
List-Help: <mailto:pve-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=subscribe>
X-List-Received-Date: Fri, 20 May 2022 07:41:13 -0000
On 19/05/2022 10:48, Oguz Bektas wrote:
> we already have the 'mtu' option in the API, so we can just expose
> that option inside the 'Advanced' menu for virtio network interfaces.
>
> Reviewed-By: Aaron Lauterer <a.lauterer@proxmox.com>
> Tested-By: Aaron Lauterer <a.lauterer@proxmox.com>
> Tested-By: Dylan Whyte <d.whyte@proxmox.com>
aren't those from two version previous with some change in between, or
did they re-test / re-review this in private?
As IMO it's not really correct to let one believe that this exact version
was tested by two people.
Functionality wise it looks Ok, a few final layout and code style nits
I'd still like to see inline.
> Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
> ---
>
> v4->v5:
> * changed default mtu from 1500 to empty (dominik's review on v4 caught
> a small bug where the mtu would be set to 1500 even without showing the
> advanced fields)
>
>
> www/manager6/qemu/NetworkEdit.js | 34 ++++++++++++++++++++++++++++++++
> 1 file changed, 34 insertions(+)
>
> diff --git a/www/manager6/qemu/NetworkEdit.js b/www/manager6/qemu/NetworkEdit.js
> index b39cffdc..91038de7 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,17 @@ Ext.define('PVE.qemu.NetworkInputPanel', {
> return params;
> },
>
> + viewModel: {
> + data: {
> + network_model: '',
nit: please avoid introducing new snake_case in the gui, we normally use camelCase here.
> + mtu: '',
> + },
> + formulas: {
> + isVirtio: (get) => get('network_model') === 'virtio',
tip, single parameter arrow functions don't need the parenthesis,
isVirtio: get => get('networkModel') === 'virtio',
> + showMTUHint: (get) => get('mtu') === 1,
showMtuHint
> + },
> + },
> +
> setNetwork: function(confid, data) {
> 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);
> @@ -130,6 +143,7 @@ Ext.define('PVE.qemu.NetworkInputPanel', {
> xtype: 'pveNetworkCardSelector',
> name: 'model',
> fieldLabel: gettext('Model'),
> + bind: '{network_model}',
> value: PVE.qemu.OSDefaults.generic.networkCard,
> allowBlank: false,
> },
> @@ -161,6 +175,26 @@ Ext.define('PVE.qemu.NetworkInputPanel', {
> value: '',
> allowBlank: true,
> },
why not add the field to advancedColumn1 and the hint in advancedColumB, so that
the form is more balanced out and the hint isn't wrapping lines increasing the
height of the row its in (even if there wasn't anything _yet_ on the left side of
it to really notice).
> + {
> + xtype: 'proxmoxintegerfield',
> + name: 'mtu',
> + fieldLabel: 'MTU',
> + bind: {
> + disabled: '{!isVirtio}',
> + value: '{mtu}',
> + },
> + minValue: 1,
> + maxValue: 65520,
> + allowBlank: true,
> + },
> + {
> + xtype: 'displayfield',
> + userCls: 'pmx-hint',
> + value: gettext("mtu=1 is a special value, the MTU value will be inherited from the current bridge"),
Maybe:
"Use the special value '1' to inherit the MTU from the underlying bridge"
> + bind: {
> + hidden: '{!showMTUHint}',
> + },
> + },
> ];
>
> me.callParent();