[pve-devel] [PATCH qemu-server 0/2] fix #4378: standardized error for ovmf files

[pve-devel] [PATCH qemu-server 0/2] fix #4378: standardized error for ovmf files

[Noel Ullreich]

* fixed the inconsistensies in the error messages for missing OVMF
files

* added a check for OVMF_VAR files so that errors are not as cryptic

Noel Ullreich (2):
  fix #4378: standardized error for ovmf files
  catch missing ovmf file

 PVE/QemuServer.pm | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

-- 
2.30.2

[pve-devel] [PATCH qemu-server 1/2] fix #4378: standardized error for ovmf files

[Noel Ullreich]

The error messages for missing OVMF_CODE and OVMF_VARS files were
inconsistent as well as the error for the missing base var file not
telling you the expected path.

Signed-off-by: Noel Ullreich <n.ullreich@proxmox.com>
---
 PVE/QemuServer.pm | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index a52a883..2a2f1f7 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -3639,7 +3639,7 @@ sub config_to_command {
 	}
 
 	my ($ovmf_code, $ovmf_vars) = get_ovmf_files($arch, $d, $q35);
-	die "uefi base image '$ovmf_code' not found\n" if ! -f $ovmf_code;
+	die "EFI base image '$ovmf_code' not found\n" if ! -f $ovmf_code;
 
 	my ($path, $format);
 	my $read_only_str = '';
@@ -8071,7 +8071,7 @@ sub get_efivars_size {
     $efidisk //= $conf->{efidisk0} ? parse_drive('efidisk0', $conf->{efidisk0}) : undef;
     my $smm = PVE::QemuServer::Machine::machine_type_is_q35($conf);
     my (undef, $ovmf_vars) = get_ovmf_files($arch, $efidisk, $smm);
-    die "uefi vars image '$ovmf_vars' not found\n" if ! -f $ovmf_vars;
+    die "EFI vars image '$ovmf_vars' not found\n" if ! -f $ovmf_vars;
     return -s $ovmf_vars;
 }
 
@@ -8099,7 +8099,7 @@ sub create_efidisk($$$$$$$) {
     my ($storecfg, $storeid, $vmid, $fmt, $arch, $efidisk, $smm) = @_;
 
     my (undef, $ovmf_vars) = get_ovmf_files($arch, $efidisk, $smm);
-    die "EFI vars default image not found\n" if ! -f $ovmf_vars;
+    die "EFI vars default image '$ovmf_vars' not found\n" if ! -f $ovmf_vars;
 
     my $vars_size_b = -s $ovmf_vars;
     my $vars_size = PVE::Tools::convert_size($vars_size_b, 'b' => 'kb');
-- 
2.30.2

[pve-devel] [PATCH qemu-server 2/2] catch missing ovmf file

[Noel Ullreich]

check to see if the OVMF_VARS file actually exists. otherwise lines
3666 and 3673 break and give a cryptic error message

Signed-off-by: Noel Ullreich <n.ullreich@proxmox.com>
---
 PVE/QemuServer.pm | 1 +
 1 file changed, 1 insertion(+)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 2a2f1f7..38f3145 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -3640,6 +3640,7 @@ sub config_to_command {
 
 	my ($ovmf_code, $ovmf_vars) = get_ovmf_files($arch, $d, $q35);
 	die "EFI base image '$ovmf_code' not found\n" if ! -f $ovmf_code;
+	die "EFI vars image '$ovmf_vars' not found\n" if ! -f $ovmf_vars;
 
 	my ($path, $format);
 	my $read_only_str = '';
-- 
2.30.2

Re: [pve-devel] [PATCH qemu-server 2/2] catch missing ovmf file

[Thomas Lamprecht]

Am 06/12/2022 um 14:11 schrieb Noel Ullreich:
> check to see if the OVMF_VARS file actually exists. otherwise lines
> 3666 and 3673 break and give a cryptic error message

I do not think that referencing lines in the commit message is helpful, rather
just describe it in general, e.g., ".. otherwise subsequent code breaks with
cryptic errors"

> 
> Signed-off-by: Noel Ullreich <n.ullreich@proxmox.com>
> ---
>  PVE/QemuServer.pm | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index 2a2f1f7..38f3145 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -3640,6 +3640,7 @@ sub config_to_command {
>  
>  	my ($ovmf_code, $ovmf_vars) = get_ovmf_files($arch, $d, $q35);
>  	die "EFI base image '$ovmf_code' not found\n" if ! -f $ovmf_code;
> +	die "EFI vars image '$ovmf_vars' not found\n" if ! -f $ovmf_vars;


why not move this check into the get_ovmf_files sub then, so that it's in a
central place? If we need to skip checking in some place (we shouldn't), one
could just add a $nocheck parameter to the helper for opting-out.

I mean, qemu-server's dependency on "pve-edk2-firmware" should already avoid
this error in the first place anyway...

>  
>  	my ($path, $format);
>  	my $read_only_str = '';