Re: [PATCH 08/21] block/backup: stricter backup_calculate_cluster_size()

[Max Reitz]

On 17.05.21 08:44, Vladimir Sementsov-Ogievskiy wrote:
> No reason to tolerate bdrv_get_info() errors except for ENOTSUP. Let's
> just error-out, it's simpler and safer.

Hm, doesn’t look that much simpler to me.  Not sure how much safer it 
is, because the point was that in the target_does_cow case, we would 
like a cluster size hint, but it isn’t necessary.  So if we don’t get 
one, regardless of the reason, we use the default cluster size.  I don’t 
know why ENOTSUP should be treated in a special way there.

So I don’t know.

Max

> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   block/backup.c | 14 +++++---------
>   1 file changed, 5 insertions(+), 9 deletions(-)
>
> diff --git a/block/backup.c b/block/backup.c
> index fe685e411b..fe7a1f1e37 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -367,7 +367,10 @@ static int64_t 
> backup_calculate_cluster_size(BlockDriverState *target,
>        * targets with a backing file, try to avoid COW if possible.
>        */
>       ret = bdrv_get_info(target, &bdi);
> -    if (ret == -ENOTSUP && !target_does_cow) {
> +    if (ret < 0 && ret != -ENOTSUP) {
> +        error_setg_errno(errp, -ret, "Failed to get target info");
> +        return ret;
> +    } else if (ret == -ENOTSUP && !target_does_cow) {
>           /* Cluster size is not defined */
>           warn_report("The target block device doesn't provide "
>                       "information about the block size and it doesn't have a "
> @@ -376,14 +379,7 @@ static int64_t 
> backup_calculate_cluster_size(BlockDriverState *target,
>                       "this default, the backup may be unusable",
>                       BACKUP_CLUSTER_SIZE_DEFAULT);
>           return BACKUP_CLUSTER_SIZE_DEFAULT;
> -    } else if (ret < 0 && !target_does_cow) {
> -        error_setg_errno(errp, -ret,
> -            "Couldn't determine the cluster size of the target image, "
> -            "which has no backing file");
> -        error_append_hint(errp,
> -            "Aborting, since this may create an unusable destination image\n");
> -        return ret;
> -    } else if (ret < 0 && target_does_cow) {
> +    } else if (ret == -ENOTSUP && target_does_cow) {
>           /* Not fatal; just trudge on ahead. */
>           return BACKUP_CLUSTER_SIZE_DEFAULT;
>       }