Re: [PATCH v2 qemu-server 08/16] api: allow setting the efi-firmware option

[Fiona Ebner <f.ebner@proxmox.com>]

Am 28.09.26 um 7:49 AM schrieb Christian Ludwig:
> Add the option to the POST/PUT {vmid}/config endpoints. It needs
> VM.Config.Options permission, like the 'bios' option it modifies, and is
> rejected without bios=ovmf.
> 
> Deleting it does not trigger volume cleanup, firmware images are shared.
> 
> Signed-off-by: Christian Ludwig <christian_ludwig@genua.de>
> ---
>  src/PVE/API2/Qemu.pm | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/src/PVE/API2/Qemu.pm b/src/PVE/API2/Qemu.pm
> index 71247eec..8b12bb53 100644
> --- a/src/PVE/API2/Qemu.pm
> +++ b/src/PVE/API2/Qemu.pm
> @@ -277,6 +277,10 @@ my $check_storage_access = sub {
>          "/storage/$settings->{vmstatestorage}",
>          ['Datastore.AllocateSpace'],
>      ) if defined($settings->{vmstatestorage});
> +
> +    PVE::Storage::check_volume_access(
> +        $rpcenv, $authuser, $storecfg, $vmid, $settings->{'efi-firmware'}, 'efi-firmware',
> +    ) if defined($settings->{'efi-firmware'});
>  };
>  
>  my $check_storage_access_clone = sub {
> @@ -825,6 +829,7 @@ my $generaloptions = {

I wonder if it's not more fitting in the $hwtypeoptions? But not sure.
My suggestion to have it be a sub-property of 'bios' would conflict with
that again.

>      'autostart' => 1,
>      'bios' => 1,
>      'description' => 1,
> +    'efi-firmware' => 1,
>      'keyboard' => 1,
>      'localtime' => 1,
>      'migrate_downtime' => 1,
> @@ -1366,6 +1371,9 @@ __PACKAGE__->register_method({
>  
>              $check_drive_param->($param, $storecfg);
>  
> +            raise_param_exc({ 'efi-firmware' => "requires bios=ovmf" })
> +                if $param->{'efi-firmware'} && ($param->{bios} // '') ne 'ovmf';
> +
>              PVE::QemuServer::Network::add_random_macs($param);
>          }
>  
> @@ -2527,6 +2535,13 @@ my $update_vm_api = sub {
>                      print "automatic pinning of machine version failed - $@" if $@;
>                  }
>                  $conf->{pending}->{$opt} = $param->{$opt};
> +            } elsif ($opt eq 'efi-firmware') {
> +                PVE::Storage::check_volume_access(
> +                    $rpcenv, $authuser, $storecfg, $vmid, $param->{$opt}, 'efi-firmware',
> +                );
> +                my $bios = $param->{bios} // $conf->{pending}->{bios} // $conf->{bios} // '';
> +                raise_param_exc({ $opt => "requires bios=ovmf" }) if $bios ne 'ovmf';
> +                $conf->{pending}->{$opt} = $param->{$opt};

This covers the case where 'efi-firmware' changes, but it could be that
the 'bios' setting is changed away from 'ovmf' later on, so you'd also
need to check in a branch for 'bios', there also for deletion. Might be
good to have a helper and call that from all relevant places. Or if
having 'bios' be a property string, it can be checked together.

>              } elsif ($opt eq 'cipassword') {
>                  if (!PVE::QemuServer::Helpers::windows_version($conf->{ostype})) {
>                      # Same logic as in cloud-init (but with the regex fixed...)

Best Regards,
Fiona