From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <f.ebner@proxmox.com>
Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68])
(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)
key-exchange X25519 server-signature RSA-PSS (2048 bits))
(No client certificate requested)
by lists.proxmox.com (Postfix) with ESMTPS id A823462E0A
for <pve-devel@lists.proxmox.com>; Wed, 23 Feb 2022 10:08:57 +0100 (CET)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
by firstgate.proxmox.com (Proxmox) with ESMTP id 9766E2C154
for <pve-devel@lists.proxmox.com>; Wed, 23 Feb 2022 10:08:27 +0100 (CET)
Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com
[94.136.29.106])
(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)
key-exchange X25519 server-signature RSA-PSS (2048 bits))
(No client certificate requested)
by firstgate.proxmox.com (Proxmox) with ESMTPS id 155D02C148
for <pve-devel@lists.proxmox.com>; Wed, 23 Feb 2022 10:08:27 +0100 (CET)
Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1])
by proxmox-new.maurer-it.com (Proxmox) with ESMTP id D0E9F46311
for <pve-devel@lists.proxmox.com>; Wed, 23 Feb 2022 10:08:20 +0100 (CET)
Message-ID: <71ba5a80-69e4-ed02-c1ef-8d5ab7e3216b@proxmox.com>
Date: Wed, 23 Feb 2022 10:08:04 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
Thunderbird/91.6.0
Content-Language: en-US
To: pve-devel@lists.proxmox.com, m.heiserer@proxmox.com
References: <20220217141251.739753-1-m.heiserer@proxmox.com>
From: Fabian Ebner <f.ebner@proxmox.com>
In-Reply-To: <20220217141251.739753-1-m.heiserer@proxmox.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
X-SPAM-LEVEL: Spam detection results: 0
AWL 0.131 Adjusted score from AWL reputation of From: address
BAYES_00 -1.9 Bayes spam probability is 0 to 1%
KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment
NICE_REPLY_A -0.001 Looks like a legit reply (A)
SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record
SPF_PASS -0.001 SPF: sender matches SPF record
T_SCC_BODY_TEXT_LINE -0.01 -
Subject: Re: [pve-devel] [PATCH qemu-server] fix 3674: QEMU restore: verify
storage allows images before writing
X-BeenThere: pve-devel@lists.proxmox.com
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Proxmox VE development discussion <pve-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pve-devel/>
List-Post: <mailto:pve-devel@lists.proxmox.com>
List-Help: <mailto:pve-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=subscribe>
X-List-Received-Date: Wed, 23 Feb 2022 09:08:57 -0000
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.