| Message ID | 20210517064428.16223-10-vsementsov@virtuozzo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block: publish backup-top filter | expand |
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; > } >
17.05.2021 19:57, Max Reitz wrote: > 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. > I'm probably OK to drop this for now and don't care. Still, I can share what brings me to this: First I thought that cluster size should be easily available for any driver: protocol drivers and not-backing-supporting format drivers can set it to 1 or to request_alignment, if they don't have a "cluster" in mind. backing-supporting format drivers should of course provide actual cluster size And I decided to just add bs->cluster_size variable, set on driver open, to simplify the whole thing and make it clean. Then, most this detect-cluster-size function would be just dropped. But it occurs, that there is one driver, that has a good and rather tricky reason for ENOTSUP: vmdk can have several extents with different cluster size.. So I give up refactored, and finished with this one patch. It can be simply dropped, I am not really a fan of it.. > >> 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; >> } >> >
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; }
No reason to tolerate bdrv_get_info() errors except for ENOTSUP. Let's just error-out, it's simpler and safer. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> --- block/backup.c | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-)