Re: [pve-devel] [PATCH qemu v2 07/21] PVE backup: add fleecing option

[Fiona Ebner <f.ebner@proxmox.com>]

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;
>> +                }