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

[pve-devel] [PATCH qemu-server v2] 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>
---
 changes from v1:
 * rebased to account for move from sub config_to_command to sub
   print_ovmf_drive_commandlines
 * left out check for existing EFI vars image in sub config_to_command
   since it was redundant

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

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index d94fe5a..cfdd2b2 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -3522,7 +3522,7 @@ my sub print_ovmf_drive_commandlines {
     my $d = $conf->{efidisk0} ? parse_drive('efidisk0', $conf->{efidisk0}) : undef;
 
     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 $var_drive_str = "if=pflash,unit=1,id=drive-efidisk0";
     if ($d) {
@@ -8070,7 +8070,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;
 }
 
@@ -8098,7 +8098,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

Re: [pve-devel] [PATCH qemu-server v2] fix #4378: standardized error for ovmf files

[Wolfgang Bumiller]

On Thu, Jan 12, 2023 at 11:57:02AM +0100, Noel Ullreich wrote:
> 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>
> ---
>  changes from v1:
>  * rebased to account for move from sub config_to_command to sub
>    print_ovmf_drive_commandlines
>  * left out check for existing EFI vars image in sub config_to_command
>    since it was redundant
> 
>  PVE/QemuServer.pm | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index d94fe5a..cfdd2b2 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -3522,7 +3522,7 @@ my sub print_ovmf_drive_commandlines {
>      my $d = $conf->{efidisk0} ? parse_drive('efidisk0', $conf->{efidisk0}) : undef;
>  
>      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;

I agree with Thomas' v1 reply that these checks can just happen once
directly in `get_ovmf_files` for both files, even if we don't always
fetch both paths, since in practice you either need both or none
available anyway, and under normal circumstances they won't be missing
anyway.

Can you please move them?

>  
>      my $var_drive_str = "if=pflash,unit=1,id=drive-efidisk0";
>      if ($d) {
> @@ -8070,7 +8070,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;
>  }
>  
> @@ -8098,7 +8098,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