Re: [pve-devel] [PATCH v2 3/12] fix #4225: qemuserver: add function to eject isofiles

[Daniel Kral <d.kral@proxmox.com>]

On 1/13/25 09:55, Daniel Herzig wrote:
> Current behaviour prevents a VM from starting, if an ISO file defined
> in the configuration becomes unavailable.
> 
> The function eject_nonrequired_isos checks on whether a cdrom drive is
> marked as 'required' or not. If the parameter 'required' is not
> defined, it will assume 'required' to be true and keep the current
> behaviour.
> 
> If 'required' is set to 0, the function 'ejects' the ISO file by
> setting the drive's file value to 'none', if the underlying storage is
> unavailable or if the defined file is unavailable for another reason.
> 
> The function is called while config_to_command iterates over all
> volumes to allow for early storage activation and early exit in the
> case of missing required files.
> 
> Signed-off-by: Daniel Herzig <d.herzig@proxmox.com>
> ---
>   PVE/QemuServer.pm | 39 +++++++++++++++++++++++++++++++++++++++
>   1 file changed, 39 insertions(+)
> 
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index d07c170e..f72878d3 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -4041,6 +4041,8 @@ sub config_to_command {
>       PVE::QemuConfig->foreach_volume($conf, sub {
>   	my ($ds, $drive) = @_;
>   
> +	eject_nonrequired_isos($ds, $drive, $vmid, $storecfg, $conf);
> +

This change will unfortunately make two config2cmd test cases fail and 
therefore the build process will also fail. It is important that the 
package can be built at each individual commit so to make the package 
bisectable.

IMO this patch could be split into "introduce eject_norequired_isos" and 
patches #4-#9 could be squashed and put together with adding 
"eject_nonrequired_isos" to config_to_command in the same patch. 
Therefore someone reviewing (now or in the future) and know what tests 
needed to be added/changed when adding this function call to 
config_to_command.

>   	if (PVE::Storage::parse_volume_id($drive->{file}, 1)) {
>   	    check_volume_storage_type($storecfg, $drive->{file});
>   	    push @$vollist, $drive->{file};
> @@ -8999,6 +9001,43 @@ sub delete_ifaces_ipams_ips {
>       }
>   }
>   
> +sub eject_nonrequired_isos {
> +    my ($ds, $drive, $vmid, $storecfg, $conf) = @_;
> +    # set 1 to exclude cloudinit. cloudinit isos are always required.
> +    if (drive_is_cdrom($drive, 1)
> +	&& $drive->{file} ne 'none'
> +	&& $drive->{file} ne 'cdrom') {

nit: IMO, this could be an early return:

return if !drive_is_cdrom($drive, 1);
return if $drive->{file} eq 'none' || $drive->{file} eq 'cdrom';

So that we can reduce the following to only 2 indentation levels.

> +	$drive->{required} = 1 if !defined($drive->{required});
> +	my $iso_volid = $drive->{file};
> +	my $iso_path = get_iso_path($storecfg, $vmid, $drive->{file});

nit: third argument could be $iso_volid

> +	my $store_err;
> +	if ($iso_volid !~ m|^/|) {
> +	    my $iso_storage  = PVE::Storage::parse_volume_id($iso_volid, 1);
> +	    eval { PVE::Storage::activate_storage($storecfg, $iso_storage); };
> +	    $store_err = $@;
> +	}
> +	if ($store_err) {
> +	    if ($drive->{required}) {
> +		die "cannot access required file: '${ds}: ${iso_volid}': ${store_err}\n";
> +	    } else {
> +		log_warn("eject '${ds}: ${iso_volid}': ${store_err}");
> +		$drive->{file} = 'none';
> +		$conf->{$ds} = print_drive($drive);
> +	    }
> +	} else {
> +	    if (!file_exists($iso_path)) {
> +		if ($drive->{required}) {
> +		    die "required file does not exist: '${ds}: ${iso_volid}'\n";
> +		} else {
> +		    log_warn("eject '${ds}: ${iso_volid}': file does not exist");
> +		    $drive->{file} = 'none';
> +		    $conf->{$ds} = print_drive($drive);
> +		}
> +	    }
> +	}

nit: the logic between an unavailable storage and an unavailable ISO 
image are very similar (both `$drive->{required} && $store_err` as well 
as `$drive->{required} && !file_exists($iso_path)` have the same exit 
control path), so we could simplify this e.g. to this (changes the 
warning message to a generic message for unavailable storages too):

if ($drive->{required}) {
     die "cannot access required file: '${ds}: ${iso_volid}': 
${store_err}\n" if $store_err;
     die "required file does not exist: '${ds}: ${iso_volid}'\n" if 
!file_exists($iso_path);
}

return if !$store_err && file_exists($iso_path);

log_warn("eject '${ds}: ${iso_volid}': storage unavailable or file does 
not exist");

$drive->{file} = 'none';
$conf->{$ds} = print_drive($drive);


> +    }
> +}
> +
>   sub file_exists {
>       my $file_path = shift;
>       return -e $file_path

Else consider this:

Reviewed-by: Daniel Kral <d.kral@proxmox.com>
Tested-by: Daniel Kral <d.kral@proxmox.com>


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel