[pve-devel] [PATCH manager 1/2] GUI: efi disk: use correct version with aarch64

[pve-devel] [PATCH manager 1/2] GUI: efi disk: use correct version with aarch64

[Matthias Heiserer]

Sets the EFI version to 2m when arch=aarch64.

When the VM has arch=aarch64, creating an EFI disk failed with
"Can't use an undefined value as an ARRAY reference at /usr/share/perl5/PVE/QemuServer.pm line 3382. (500)"

That's because we only have EFI 2m available for aarch64.

Reported in the forum: https://forum.proxmox.com/threads/121160/

Signed-off-by: Matthias Heiserer <m.heiserer@proxmox.com>
---
 www/manager6/qemu/HDEfi.js | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/www/manager6/qemu/HDEfi.js b/www/manager6/qemu/HDEfi.js
index a8ca8f56..ef3092a6 100644
--- a/www/manager6/qemu/HDEfi.js
+++ b/www/manager6/qemu/HDEfi.js
@@ -26,7 +26,7 @@ Ext.define('PVE.qemu.EFIDiskInputPanel', {
 
 	// always default to newer 4m type with secure boot support, if we're
 	// adding a new EFI disk there can't be any old state anyway
-	me.drive.efitype = '4m';
+	me.drive.efitype = me.config.useOldVersion ? '2m' : '4m';
 	me.drive['pre-enrolled-keys'] = values.preEnrolledKeys;
 	delete values.preEnrolledKeys;
 
@@ -103,15 +103,24 @@ Ext.define('PVE.qemu.EFIDiskEdit', {
 	    throw "no node name specified";
 	}
 
-	me.items = [{
+	const efiInputPanel = Ext.create({
 	    xtype: 'pveEFIDiskInputPanel',
 	    onlineHelp: 'qm_bios_and_uefi',
 	    confid: me.confid,
 	    nodename: nodename,
 	    usesEFI: me.usesEFI,
 	    isCreate: true,
-	}];
+	    useOldVersion: false,
+	});
+	me.items = [efiInputPanel];
 
 	me.callParent();
+
+	me.load({
+	    success: function(response) {
+		const data = response.result.data;
+		efiInputPanel.config.useOldVersion = data.arch === 'aarch64';
+	    },
+	});
     },
 });
-- 
2.30.2

[pve-devel] [PATCH manager 2/2] GUI: efi disk: cleanup - var to const

[Matthias Heiserer]

Signed-off-by: Matthias Heiserer <m.heiserer@proxmox.com>
---
 www/manager6/qemu/HDEfi.js | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/www/manager6/qemu/HDEfi.js b/www/manager6/qemu/HDEfi.js
index ef3092a6..f6b7fc26 100644
--- a/www/manager6/qemu/HDEfi.js
+++ b/www/manager6/qemu/HDEfi.js
@@ -9,13 +9,13 @@ Ext.define('PVE.qemu.EFIDiskInputPanel', {
     vmconfig: {}, // used to select usused disks
 
     onGetValues: function(values) {
-	var me = this;
+	const me = this;
 
 	if (me.disabled) {
 	    return {};
 	}
 
-	var confid = 'efidisk0';
+	const confid = 'efidisk0';
 
 	if (values.hdimage) {
 	    me.drive.file = values.hdimage;
@@ -37,7 +37,7 @@ Ext.define('PVE.qemu.EFIDiskInputPanel', {
     },
 
     setNodename: function(nodename) {
-	var me = this;
+	const me = this;
 	me.down('#hdstorage').setNodename(nodename);
 	me.down('#hdimage').setStorage(undefined, nodename);
     },
@@ -50,7 +50,7 @@ Ext.define('PVE.qemu.EFIDiskInputPanel', {
     },
 
     initComponent: function() {
-	var me = this;
+	const me = this;
 
 	me.drive = {};
 
@@ -96,9 +96,9 @@ Ext.define('PVE.qemu.EFIDiskEdit', {
 
     width: 450,
     initComponent: function() {
-	var me = this;
+	const me = this;
 
-	var nodename = me.pveSelNode.data.node;
+	const nodename = me.pveSelNode.data.node;
 	if (!nodename) {
 	    throw "no node name specified";
 	}
-- 
2.30.2

Re: [pve-devel] [PATCH manager 2/2] GUI: efi disk: cleanup - var to const

[Thomas Lamprecht]

Am 17/01/2023 um 13:17 schrieb Matthias Heiserer:
> Signed-off-by: Matthias Heiserer <m.heiserer@proxmox.com>
> ---
>  www/manager6/qemu/HDEfi.js | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/www/manager6/qemu/HDEfi.js b/www/manager6/qemu/HDEfi.js
> index ef3092a6..f6b7fc26 100644
> --- a/www/manager6/qemu/HDEfi.js
> +++ b/www/manager6/qemu/HDEfi.js
> @@ -9,13 +9,13 @@ Ext.define('PVE.qemu.EFIDiskInputPanel', {
>      vmconfig: {}, // used to select usused disks
>  
>      onGetValues: function(values) {
> -	var me = this;
> +	const me = this;
>  
>  	if (me.disabled) {
>  	    return {};
>  	}
>  
> -	var confid = 'efidisk0';
> +	const confid = 'efidisk0';
>  
>  	if (values.hdimage) {
>  	    me.drive.file = values.hdimage;
> @@ -37,7 +37,7 @@ Ext.define('PVE.qemu.EFIDiskInputPanel', {
>      },
>  
>      setNodename: function(nodename) {
> -	var me = this;
> +	const me = this;
>  	me.down('#hdstorage').setNodename(nodename);
>  	me.down('#hdimage').setStorage(undefined, nodename);
>      },
> @@ -50,7 +50,7 @@ Ext.define('PVE.qemu.EFIDiskInputPanel', {
>      },
>  
>      initComponent: function() {
> -	var me = this;
> +	const me = this;
>  
>  	me.drive = {};
>  
> @@ -96,9 +96,9 @@ Ext.define('PVE.qemu.EFIDiskEdit', {
>  
>      width: 450,
>      initComponent: function() {
> -	var me = this;
> +	const me = this;
>  
> -	var nodename = me.pveSelNode.data.node;
> +	const nodename = me.pveSelNode.data.node;
>  	if (!nodename) {
>  	    throw "no node name specified";
>  	}

I'm not that huge fan of applying JavaScript's const broadly to non-scalar values,
as it's only a shallow constant and immutable; so no effect on array/object member.

I mean we can start to use it more for scalar variables, as eslint catches overriding
the const variable itself directly - but it probably makes sense to add some basic
rules w.r.t. when/how it should be used over `let` in our JS style guide, something
along the line of:

"Using `let` is fine in most context, and while `const` can have some advantages and
possibly even avoid some specific bugs, it should only be applied to scalar values but
not objects or arrays."

What do you think (apart from grammar/typos ;-))?

Re: [pve-devel] [PATCH manager 1/2] GUI: efi disk: use correct version with aarch64

[Thomas Lamprecht]

for subject: s/GUI/ui/ to better match the predominantly used one.

Am 17/01/2023 um 13:17 schrieb Matthias Heiserer:
> Sets the EFI version to 2m when arch=aarch64.
> 
> When the VM has arch=aarch64, creating an EFI disk failed with
> "Can't use an undefined value as an ARRAY reference at /usr/share/perl5/PVE/QemuServer.pm line 3382. (500)"
> 
> That's because we only have EFI 2m available for aarch64.
> 
> Reported in the forum: https://forum.proxmox.com/threads/121160/
> 

If we go this route I'd also enforce using the correct one when changing
or creating VMs via the API.

Two possible alternatives:
- auto-select the existing one in the backend; drawback: if we ever add
  another size for the AAVMF image we need to take extra/special care to
  avoid breaking old systems.

- build also an AAVMF image with 4MB, but one would need to check if this
  is really possible in the first place or if there are other drawbacks.

Re: [pve-devel] [PATCH manager 2/2] GUI: efi disk: cleanup - var to const

[Matthias Heiserer]

On 18.01.2023 15:03, Thomas Lamprecht wrote:
> Am 17/01/2023 um 13:17 schrieb Matthias Heiserer:
>> Signed-off-by: Matthias Heiserer <m.heiserer@proxmox.com>
>> ---
>>   www/manager6/qemu/HDEfi.js | 12 ++++++------
>>   1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/www/manager6/qemu/HDEfi.js b/www/manager6/qemu/HDEfi.js
>> index ef3092a6..f6b7fc26 100644
>> --- a/www/manager6/qemu/HDEfi.js
>> +++ b/www/manager6/qemu/HDEfi.js
>> @@ -9,13 +9,13 @@ Ext.define('PVE.qemu.EFIDiskInputPanel', {
>>       vmconfig: {}, // used to select usused disks
>>   
>>       onGetValues: function(values) {
>> -	var me = this;
>> +	const me = this;
>>   
>>   	if (me.disabled) {
>>   	    return {};
>>   	}
>>   
>> -	var confid = 'efidisk0';
>> +	const confid = 'efidisk0';
>>   
>>   	if (values.hdimage) {
>>   	    me.drive.file = values.hdimage;
>> @@ -37,7 +37,7 @@ Ext.define('PVE.qemu.EFIDiskInputPanel', {
>>       },
>>   
>>       setNodename: function(nodename) {
>> -	var me = this;
>> +	const me = this;
>>   	me.down('#hdstorage').setNodename(nodename);
>>   	me.down('#hdimage').setStorage(undefined, nodename);
>>       },
>> @@ -50,7 +50,7 @@ Ext.define('PVE.qemu.EFIDiskInputPanel', {
>>       },
>>   
>>       initComponent: function() {
>> -	var me = this;
>> +	const me = this;
>>   
>>   	me.drive = {};
>>   
>> @@ -96,9 +96,9 @@ Ext.define('PVE.qemu.EFIDiskEdit', {
>>   
>>       width: 450,
>>       initComponent: function() {
>> -	var me = this;
>> +	const me = this;
>>   
>> -	var nodename = me.pveSelNode.data.node;
>> +	const nodename = me.pveSelNode.data.node;
>>   	if (!nodename) {
>>   	    throw "no node name specified";
>>   	}
> 
> I'm not that huge fan of applying JavaScript's const broadly to non-scalar values,
> as it's only a shallow constant and immutable; so no effect on array/object member.

I quite like const, especially with `const me = this`, because it tells 
me that I don't have to worry about the variable being reassigned.
But as long as there's a guideline on whether to use it, I'm happy :)
> 
> I mean we can start to use it more for scalar variables, as eslint catches overriding
> the const variable itself directly - but it probably makes sense to add some basic
> rules w.r.t. when/how it should be used over `let` in our JS style guide, something
> along the line of:
> 
> "Using `let` is fine in most context, and while `const` can have some advantages and
> possibly even avoid some specific bugs, it should only be applied to scalar values but
> not objects or arrays."
> 
> What do you think (apart from grammar/typos ;-))?

Looks good

Re: [pve-devel] [PATCH manager 1/2] GUI: efi disk: use correct version with aarch64

[Matthias Heiserer]

On 18.01.2023 15:07, Thomas Lamprecht wrote:
> for subject: s/GUI/ui/ to better match the predominantly used one.
> 
> Am 17/01/2023 um 13:17 schrieb Matthias Heiserer:
>> Sets the EFI version to 2m when arch=aarch64.
>>
>> When the VM has arch=aarch64, creating an EFI disk failed with
>> "Can't use an undefined value as an ARRAY reference at /usr/share/perl5/PVE/QemuServer.pm line 3382. (500)"
>>
>> That's because we only have EFI 2m available for aarch64.
>>
>> Reported in the forum: https://forum.proxmox.com/threads/121160/
>>
> 
> If we go this route I'd also enforce using the correct one when changing
> or creating VMs via the API.
When creating via the API (and not setting a type) the default is used, 
which works with aarch64. Only if 4m is explicitly set, it errors.
The ui user can't set the version, that's why I changed it there.

> 
> Two possible alternatives:
> - auto-select the existing one in the backend; drawback: if we ever add
>    another size for the AAVMF image we need to take extra/special care to
>    avoid breaking old systems. >
> - build also an AAVMF image with 4MB, but one would need to check if this
>    is really possible in the first place or if there are other drawbacks.
To me, the second option seems better, should it work.
I don't like the idea of letting the user provide a parameter and 
silently ignoring it.

Re: [pve-devel] [PATCH manager 1/2] GUI: efi disk: use correct version with aarch64

[Thomas Lamprecht]

Am 20/01/2023 um 13:56 schrieb Matthias Heiserer:
> On 18.01.2023 15:07, Thomas Lamprecht wrote:
>> for subject: s/GUI/ui/ to better match the predominantly used one.
>>
>> Am 17/01/2023 um 13:17 schrieb Matthias Heiserer:
>>> Sets the EFI version to 2m when arch=aarch64.
>>>
>>> When the VM has arch=aarch64, creating an EFI disk failed with
>>> "Can't use an undefined value as an ARRAY reference at /usr/share/perl5/PVE/QemuServer.pm line 3382. (500)"
>>>
>>> That's because we only have EFI 2m available for aarch64.
>>>
>>> Reported in the forum: https://forum.proxmox.com/threads/121160/
>>>
>>
>> If we go this route I'd also enforce using the correct one when changing
>> or creating VMs via the API.
> When creating via the API (and not setting a type) the default is used, which works with aarch64. Only if 4m is explicitly set, it errors.
> The ui user can't set the version, that's why I changed it there.

but your quoted error makes it obvious that the backend doesn't really
catches the illegal combination explicitly, or am I'm missing something?

> 
>>
>> Two possible alternatives:
>> - auto-select the existing one in the backend; drawback: if we ever add
>>    another size for the AAVMF image we need to take extra/special care to
>>    avoid breaking old systems. >
>> - build also an AAVMF image with 4MB, but one would need to check if this
>>    is really possible in the first place or if there are other drawbacks.
> To me, the second option seems better, should it work.
> I don't like the idea of letting the user provide a parameter and silently ignoring it.

yeah, I'd favor that one too

Re: [pve-devel] [PATCH manager 2/2] GUI: efi disk: cleanup - var to const

[Thomas Lamprecht]

Am 20/01/2023 um 12:16 schrieb Matthias Heiserer:
>>
>> I'm not that huge fan of applying JavaScript's const broadly to non-scalar values,
>> as it's only a shallow constant and immutable; so no effect on array/object member.
> 
> I quite like const, especially with `const me = this`, because it tells me that I don't have to worry about the variable being reassigned.
> But as long as there's a guideline on whether to use it, I'm happy 😄

Yeah for the `me` (component) this alias it could have some merit;
albeit I'm don't think this was ever an issue in the committed code,
neither during development, but for the latter I naturally can only
speak for myself.

Re: [pve-devel] [PATCH manager 1/2] GUI: efi disk: use correct version with aarch64

[Matthias Heiserer]

On 20.01.2023 14:17, Thomas Lamprecht wrote:
> Am 20/01/2023 um 13:56 schrieb Matthias Heiserer:
>> On 18.01.2023 15:07, Thomas Lamprecht wrote:
>>> for subject: s/GUI/ui/ to better match the predominantly used one.
>>>
>>> Am 17/01/2023 um 13:17 schrieb Matthias Heiserer:
>>>> Sets the EFI version to 2m when arch=aarch64.
>>>>
>>>> When the VM has arch=aarch64, creating an EFI disk failed with
>>>> "Can't use an undefined value as an ARRAY reference at /usr/share/perl5/PVE/QemuServer.pm line 3382. (500)"
>>>>
>>>> That's because we only have EFI 2m available for aarch64.
>>>>
>>>> Reported in the forum: https://forum.proxmox.com/threads/121160/
>>>>
>>>
>>> If we go this route I'd also enforce using the correct one when changing
>>> or creating VMs via the API.
>> When creating via the API (and not setting a type) the default is used, which works with aarch64. Only if 4m is explicitly set, it errors.
>> The ui user can't set the version, that's why I changed it there.
> 
> but your quoted error makes it obvious that the backend doesn't really
> catches the illegal combination explicitly, or am I'm missing something?
> 
Sorry, maybe I misunderstood you.
Yes, the backend fails with a worse message than it should, but it does 
fail.
Changing the error wouldn't hurt though.


>>
>>>
>>> Two possible alternatives:
>>> - auto-select the existing one in the backend; drawback: if we ever add
>>>     another size for the AAVMF image we need to take extra/special care to
>>>     avoid breaking old systems. >
>>> - build also an AAVMF image with 4MB, but one would need to check if this
>>>     is really possible in the first place or if there are other drawbacks.
>> To me, the second option seems better, should it work.
>> I don't like the idea of letting the user provide a parameter and silently ignoring it.
> 
> yeah, I'd favor that one too
I'll have a look at it