From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: 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 ; 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 ; 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 ; 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 ; 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 Cc: pve-devel@lists.proxmox.com References: <20240315102502.84163-1-f.ebner@proxmox.com> <20240315102502.84163-8-f.ebner@proxmox.com> Content-Language: en-US From: Fiona Ebner In-Reply-To: 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-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; >> + }