[pve-devel] [PATCH qemu-server] fix 3674: QEMU restore: verify storage allows images before writing

[pve-devel] [PATCH qemu-server] fix 3674: QEMU restore: verify storage allows images before writing

[Matthias Heiserer]

When restoring a backup and the storage the disks would be created on
doesn't allow 'images', the process errors without cleanup.
This is the same behaviour we currently have when the storage is
disabled.

Signed-off-by: Matthias Heiserer <m.heiserer@proxmox.com>
---
 PVE/QemuServer.pm | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index a99f1a5..2a1ec48 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -6299,6 +6299,10 @@ my $restore_allocate_devices = sub {
 	my $supported = grep { $_ eq $d->{format} } @$validFormats;
 	$d->{format} = $defFormat if !$supported;
 
+	# check if images can be stored on the requested storage
+	die "Content type 'images' is not available on storage '$storeid'\n"
+	    if !$scfg->{content}->{images};
+
 	my $name;
 	if ($d->{is_cloudinit}) {
 	    $name = "vm-$vmid-cloudinit";
-- 
2.30.2

Re: [pve-devel] [PATCH qemu-server] fix 3674: QEMU restore: verify storage allows images before writing

[Matthias Heiserer]

Sorry, wrong bug. Should be 3886.

Re: [pve-devel] [PATCH qemu-server] fix 3674: QEMU restore: verify storage allows images before writing

[Fabian Ebner]

Am 17.02.22 um 15:12 schrieb Matthias Heiserer:
> When restoring a backup and the storage the disks would be created on
> doesn't allow 'images', the process errors without cleanup.
> This is the same behaviour we currently have when the storage is
> disabled.
> 
> Signed-off-by: Matthias Heiserer <m.heiserer@proxmox.com>
> ---
>  PVE/QemuServer.pm | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index a99f1a5..2a1ec48 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -6299,6 +6299,10 @@ my $restore_allocate_devices = sub {
>  	my $supported = grep { $_ eq $d->{format} } @$validFormats;
>  	$d->{format} = $defFormat if !$supported;
>  
> +	# check if images can be stored on the requested storage

Nit: The comment isn't needed IMHO, because the code is pretty clear on
its own.

> +	die "Content type 'images' is not available on storage '$storeid'\n"
> +	    if !$scfg->{content}->{images};
> +
>  	my $name;
>  	if ($d->{is_cloudinit}) {
>  	    $name = "vm-$vmid-cloudinit";

Nothing wrong with the patch (except for the bug number as you already
pointed out ;)), it's just that the permission check for accessing the
storage is currently done in parse_backup_hints(), so it might be a bit
cleaner to add the new check there too. Like that, both checks are in
one place and we can abort early, before starting to allocate any disks.

It seems that there's another small bug in parse_backup_hints(), because
there's no permission check for a cloudinit disk. Would be nice if you
could fix that too.

Re: [pve-devel] [PATCH qemu-server] fix 3674: QEMU restore: verify storage allows images before writing

[Markus Frank]

With this patch restoring a backup on a disabled storage results in an
error and just creates a VM with the configuration from the backup
without any virtual disk instead of forcing the creation of the disk(s).
Works as intended.

Tested-by: Markus Frank <m.frank@proxmox.com>

On 2/17/22 15:12, Matthias Heiserer wrote:
> When restoring a backup and the storage the disks would be created on
> doesn't allow 'images', the process errors without cleanup.
> This is the same behaviour we currently have when the storage is
> disabled.
>
> Signed-off-by: Matthias Heiserer <m.heiserer@proxmox.com>
> ---
>   PVE/QemuServer.pm | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index a99f1a5..2a1ec48 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -6299,6 +6299,10 @@ my $restore_allocate_devices = sub {
>   	my $supported = grep { $_ eq $d->{format} } @$validFormats;
>   	$d->{format} = $defFormat if !$supported;
>   
> +	# check if images can be stored on the requested storage
> +	die "Content type 'images' is not available on storage '$storeid'\n"
> +	    if !$scfg->{content}->{images};
> +
>   	my $name;
>   	if ($d->{is_cloudinit}) {
>   	    $name = "vm-$vmid-cloudinit";