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) server-digest SHA256)
(No client certificate requested)
by lists.proxmox.com (Postfix) with ESMTPS id D511293EBE
for <pve-devel@lists.proxmox.com>; Wed, 10 Apr 2024 11:31:04 +0200 (CEST)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
by firstgate.proxmox.com (Proxmox) with ESMTP id B3DE8AE29
for <pve-devel@lists.proxmox.com>; Wed, 10 Apr 2024 11:31:04 +0200 (CEST)
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
for <pve-devel@lists.proxmox.com>; Wed, 10 Apr 2024 11:31:00 +0200 (CEST)
Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1])
by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 3A71843AAA
for <pve-devel@lists.proxmox.com>; Wed, 10 Apr 2024 11:31:00 +0200 (CEST)
Message-ID: <553f7d33-d739-49d0-a65e-b572d3985da2@proxmox.com>
Date: Wed, 10 Apr 2024 11:30:59 +0200
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
To: Wolfgang Bumiller <w.bumiller@proxmox.com>
Cc: pve-devel@lists.proxmox.com
References: <20240315102502.84163-1-f.ebner@proxmox.com>
<20240315102502.84163-8-f.ebner@proxmox.com>
<pbokhbft2wbc2jnthp75gzz5u3vgswr7irlarwxadvc5cr777q@avaj6rwkxugb>
Content-Language: en-US
From: Fiona Ebner <f.ebner@proxmox.com>
In-Reply-To: <pbokhbft2wbc2jnthp75gzz5u3vgswr7irlarwxadvc5cr777q@avaj6rwkxugb>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
X-SPAM-LEVEL: Spam detection results: 0
AWL -0.073 Adjusted score from AWL reputation of From: address
BAYES_00 -1.9 Bayes spam probability is 0 to 1%
DMARC_MISSING 0.1 Missing DMARC policy
KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment
SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record
SPF_PASS -0.001 SPF: sender matches SPF record
Subject: Re: [pve-devel] [PATCH qemu v2 07/21] PVE backup: add fleecing
option
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, 10 Apr 2024 09:31:04 -0000
Am 08.04.24 um 14:45 schrieb Wolfgang Bumiller:
> On Fri, Mar 15, 2024 at 11:24:48AM +0100, Fiona Ebner wrote:
>> @@ -581,6 +682,14 @@ static void create_backup_jobs_bh(void *opaque) {
>> aio_co_enter(data->ctx, data->co);
>> }
>>
>> +/*
>> + * EFI disk and TPM state are small and it's just not worth setting up fleecing for them.
>> + */
>> +static bool device_uses_fleecing(const char *device_id)
>
> Do we really want this?
>
> IMO we already have enough code trying to distinguish "real" disks from
> efidisks and tpmstate files.
>
> AFAICT we do check whether the hmp command to *create* the fleecing
> drives actually works, so... (see below)
>
>> +{
>> + return strncmp(device_id, "drive-efidisk", 13) && strncmp(device_id, "drive-tpmstate", 14);
>> +}
>> +
>> /*
>> * Returns a list of device infos, which needs to be freed by the caller. In
>> * case of an error, errp will be set, but the returned value might still be a
>> @@ -588,6 +697,7 @@ static void create_backup_jobs_bh(void *opaque) {
>> */
>> static GList coroutine_fn *get_device_info(
>> const char *devlist,
>> + bool fleecing,
>> Error **errp)
>> {
>> gchar **devs = NULL;
>> @@ -611,6 +721,31 @@ static GList coroutine_fn *get_device_info(
>> }
>> PVEBackupDevInfo *di = g_new0(PVEBackupDevInfo, 1);
>> di->bs = bs;
>> +
>> + if (fleecing && device_uses_fleecing(*d)) {
>> + g_autofree gchar *fleecing_devid = g_strconcat(*d, "-fleecing", NULL);
>> + BlockBackend *fleecing_blk = blk_by_name(fleecing_devid);
>> + if (!fleecing_blk) {
>
> ...so instead of this, we could just treat the absence of a fleecing
> BlockBackend *not* as an error, but as deliberate?
>
Yes, we could. But the check gives protection against potential (future)
bugs where we can't find the fleecing device for some other reason.
Without the check, we'd just quietly continue and it would be hard to
notice that something is wrong. So I'm slightly in favor of keeping it.
If you still want me to remove it, I'll do it in v3.
>> + error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
>> + "Device '%s' not found", fleecing_devid);
>> + goto err;
>> + }