[pve-devel] [PATCH qemu-server] fix #5284: diallow moving vm disks to storages not meant for images

[Thomas Lamprecht]

Am 29/08/2024 um 16:26 schrieb Daniel Kral:
> Adds a check if the target storage of a VM disk move between two
> different storages supports the content type 'images'. Without the
> check, it will move the disk image to storages which do not support VM
> images, which causes the VM to fail at startup (for any non-unused
> disks).
> 
> Signed-off-by: Daniel Kral <d.kral at proxmox.com>
> ---
> There is a content type check for any used volume (e.g. 'scsi0', ...) at
> PVE/QemuServer.pm:3964 in the subroutine `config_to_command` which will
> (at least) make the VM fail to start unless the volumes are moved to a
> storage that supports images again.
> 
> Also, just as a note, moving the disk to storages that do not support
> the `vdisk_alloc` method in the storage plugin (like PBS) also
> rightfully fail before and after this change.
> 
>  PVE/API2/Qemu.pm | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
> index d25a79fe..b6ba9d21 100644
> --- a/PVE/API2/Qemu.pm
> +++ b/PVE/API2/Qemu.pm
> @@ -4409,6 +4409,9 @@ __PACKAGE__->register_method({
>  	    );
>  	} elsif ($storeid) {
>  	    $rpcenv->check($authuser, "/storage/$storeid", ['Datastore.AllocateSpace']);
> +	    my $scfg = PVE::Storage::storage_config($storecfg, $storeid);
> +	    raise_param_exc({ storage => "storage '$storeid' does not support vm images" })
> +		if !$scfg->{content}->{images};

Hmm, code wise it looks OK, but not so sure if this is the best place
to check, I'd rather look into either moving this into the $load_and_check_move
closure or into the PVE::QemuServer::clone_disk method, avoiding such an issue
for all other call sites too, albeit one would need to evaluate those call sites
if it does not break an existing usecase when disallowing this everywhere.

btw. did you check if containers are also affected by this bug?

>  
>  	    $load_and_check_move->(); # early checks before forking/locking
>