This is a change in behavior, so all instances need a good
justification. The comments added here should explain my reasoning.
qed already had a comment that suggests it always expected
bdrv_truncate()/blk_truncate() to behave as if exact=true were passed
(c743849bee7 came eight months before 55b949c8476), so it was simply
broken until now.
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
block/parallels.c | 11 +++++++++--
block/qcow2.c | 6 +++++-
block/qed.c | 2 +-
qemu-img.c | 7 ++++++-
qemu-io-cmds.c | 7 ++++++-
5 files changed, 27 insertions(+), 6 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index a1a92c97a4..603211f83c 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -487,7 +487,12 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
res->leaks += count;
if (fix & BDRV_FIX_LEAKS) {
Error *local_err = NULL;
- ret = bdrv_truncate(bs->file, res->image_end_offset, false,
+
+ /*
+ * In order to really repair the image, we must shrink it.
+ * That means we have to pass exact=true.
+ */
+ ret = bdrv_truncate(bs->file, res->image_end_offset, true,
PREALLOC_MODE_OFF, &local_err);
if (ret < 0) {
error_report_err(local_err);
@@ -880,7 +885,9 @@ static void parallels_close(BlockDriverState *bs)
if ((bs->open_flags & BDRV_O_RDWR) && !(bs->open_flags & BDRV_O_INACTIVE)) {
s->header->inuse = 0;
parallels_update_header(bs);
- bdrv_truncate(bs->file, s->data_end << BDRV_SECTOR_BITS, false,
+
+ /* errors are ignored, so we might as well pass exact=true */
+ bdrv_truncate(bs->file, s->data_end << BDRV_SECTOR_BITS, true,
PREALLOC_MODE_OFF, NULL);
}
diff --git a/block/qcow2.c b/block/qcow2.c
index 4ef19dd29a..eba165de7f 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -5057,7 +5057,11 @@ static int qcow2_amend_options(BlockDriverState *bs, QemuOpts *opts,
return ret;
}
- ret = blk_truncate(blk, new_size, false, PREALLOC_MODE_OFF, errp);
+ /*
+ * Amending image options should ensure that the image has
+ * exactly the given new values, so pass exact=true here.
+ */
+ ret = blk_truncate(blk, new_size, true, PREALLOC_MODE_OFF, errp);
blk_unref(blk);
if (ret < 0) {
return ret;
diff --git a/block/qed.c b/block/qed.c
index 7c2a65af40..8005cfc305 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -674,7 +674,7 @@ static int coroutine_fn bdrv_qed_co_create(BlockdevCreateOptions *opts,
l1_size = header.cluster_size * header.table_size;
/* File must start empty and grow, check truncate is supported */
- ret = blk_truncate(blk, 0, false, PREALLOC_MODE_OFF, errp);
+ ret = blk_truncate(blk, 0, true, PREALLOC_MODE_OFF, errp);
if (ret < 0) {
goto out;
}
diff --git a/qemu-img.c b/qemu-img.c
index f8694f4f72..a3169b6113 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -3823,7 +3823,12 @@ static int img_resize(int argc, char **argv)
}
}
- ret = blk_truncate(blk, total_size, false, prealloc, &err);
+ /*
+ * The user expects the image to have the desired size after
+ * resizing, so pass @exact=true. It is of no use to report
+ * success when the image has not actually been resized.
+ */
+ ret = blk_truncate(blk, total_size, true, prealloc, &err);
if (ret < 0) {
error_report_err(err);
goto out;
diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
index 5e9017c979..1b7e700020 100644
--- a/qemu-io-cmds.c
+++ b/qemu-io-cmds.c
@@ -1710,7 +1710,12 @@ static int truncate_f(BlockBackend *blk, int argc, char **argv)
return offset;
}
- ret = blk_truncate(blk, offset, false, PREALLOC_MODE_OFF, &local_err);
+ /*
+ * qemu-io is a debugging tool, so let us be strict here and pass
+ * exact=true. It is better to err on the "emit more errors" side
+ * than to be overly permissive.
+ */
+ ret = blk_truncate(blk, offset, true, PREALLOC_MODE_OFF, &local_err);
if (ret < 0) {
error_report_err(local_err);
return ret;
--
2.21.0
On Wed, 2019-09-18 at 11:51 +0200, Max Reitz wrote:
> This is a change in behavior, so all instances need a good
> justification. The comments added here should explain my reasoning.
>
> qed already had a comment that suggests it always expected
> bdrv_truncate()/blk_truncate() to behave as if exact=true were passed
> (c743849bee7 came eight months before 55b949c8476), so it was simply
> broken until now.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> block/parallels.c | 11 +++++++++--
> block/qcow2.c | 6 +++++-
> block/qed.c | 2 +-
> qemu-img.c | 7 ++++++-
> qemu-io-cmds.c | 7 ++++++-
> 5 files changed, 27 insertions(+), 6 deletions(-)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index a1a92c97a4..603211f83c 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -487,7 +487,12 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
> res->leaks += count;
> if (fix & BDRV_FIX_LEAKS) {
> Error *local_err = NULL;
> - ret = bdrv_truncate(bs->file, res->image_end_offset, false,
> +
> + /*
> + * In order to really repair the image, we must shrink it.
> + * That means we have to pass exact=true.
> + */
> + ret = bdrv_truncate(bs->file, res->image_end_offset, true,
> PREALLOC_MODE_OFF, &local_err);
I am not familiar with the parallels format, but in theory,
a partial fix is better that nothing.
> if (ret < 0) {
> error_report_err(local_err);
> @@ -880,7 +885,9 @@ static void parallels_close(BlockDriverState *bs)
> if ((bs->open_flags & BDRV_O_RDWR) && !(bs->open_flags & BDRV_O_INACTIVE)) {
> s->header->inuse = 0;
> parallels_update_header(bs);
> - bdrv_truncate(bs->file, s->data_end << BDRV_SECTOR_BITS, false,
> +
> + /* errors are ignored, so we might as well pass exact=true */
> + bdrv_truncate(bs->file, s->data_end << BDRV_SECTOR_BITS, true,
> PREALLOC_MODE_OFF, NULL);
Fair enough.
> }
>
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 4ef19dd29a..eba165de7f 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -5057,7 +5057,11 @@ static int qcow2_amend_options(BlockDriverState *bs, QemuOpts *opts,
> return ret;
> }
>
> - ret = blk_truncate(blk, new_size, false, PREALLOC_MODE_OFF, errp);
> + /*
> + * Amending image options should ensure that the image has
> + * exactly the given new values, so pass exact=true here.
> + */
Agree.
> + ret = blk_truncate(blk, new_size, true, PREALLOC_MODE_OFF, errp);
> blk_unref(blk);
> if (ret < 0) {
> return ret;
> diff --git a/block/qed.c b/block/qed.c
> index 7c2a65af40..8005cfc305 100644
> --- a/block/qed.c
> +++ b/block/qed.c
> @@ -674,7 +674,7 @@ static int coroutine_fn bdrv_qed_co_create(BlockdevCreateOptions *opts,
> l1_size = header.cluster_size * header.table_size;
>
> /* File must start empty and grow, check truncate is supported */
I would update the above comment, with something like
"QED format requires the underlying file to have the exact expected length,
which is 0 on creation"
Or something similar.
> - ret = blk_truncate(blk, 0, false, PREALLOC_MODE_OFF, errp);
> + ret = blk_truncate(blk, 0, true, PREALLOC_MODE_OFF, errp);
> if (ret < 0) {
> goto out;
> }
> diff --git a/qemu-img.c b/qemu-img.c
> index f8694f4f72..a3169b6113 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -3823,7 +3823,12 @@ static int img_resize(int argc, char **argv)
> }
> }
>
> - ret = blk_truncate(blk, total_size, false, prealloc, &err);
> + /*
> + * The user expects the image to have the desired size after
> + * resizing, so pass @exact=true. It is of no use to report
> + * success when the image has not actually been resized.
> + */
Agree.
> + ret = blk_truncate(blk, total_size, true, prealloc, &err);
> if (ret < 0) {
> error_report_err(err);
> goto out;
> diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
> index 5e9017c979..1b7e700020 100644
> --- a/qemu-io-cmds.c
> +++ b/qemu-io-cmds.c
> @@ -1710,7 +1710,12 @@ static int truncate_f(BlockBackend *blk, int argc, char **argv)
> return offset;
> }
>
> - ret = blk_truncate(blk, offset, false, PREALLOC_MODE_OFF, &local_err);
> + /*
> + * qemu-io is a debugging tool, so let us be strict here and pass
> + * exact=true. It is better to err on the "emit more errors" side
> + * than to be overly permissive.
> + */
Also agree.
> + ret = blk_truncate(blk, offset, true, PREALLOC_MODE_OFF, &local_err);
> if (ret < 0) {
> error_report_err(local_err);
> return ret;
Other than few nitpicks which probably aren't important,
Reviewed-by: Maxim Levitsky <mlevitsky@redhat.com>
Best regards,
Maxim Levitsky
On 18.09.19 22:52, Maxim Levitsky wrote: > On Wed, 2019-09-18 at 11:51 +0200, Max Reitz wrote: >> This is a change in behavior, so all instances need a good >> justification. The comments added here should explain my reasoning. >> >> qed already had a comment that suggests it always expected >> bdrv_truncate()/blk_truncate() to behave as if exact=true were passed >> (c743849bee7 came eight months before 55b949c8476), so it was simply >> broken until now. >> >> Signed-off-by: Max Reitz <mreitz@redhat.com> >> --- >> block/parallels.c | 11 +++++++++-- >> block/qcow2.c | 6 +++++- >> block/qed.c | 2 +- >> qemu-img.c | 7 ++++++- >> qemu-io-cmds.c | 7 ++++++- >> 5 files changed, 27 insertions(+), 6 deletions(-) [...] >> diff --git a/block/qed.c b/block/qed.c >> index 7c2a65af40..8005cfc305 100644 >> --- a/block/qed.c >> +++ b/block/qed.c >> @@ -674,7 +674,7 @@ static int coroutine_fn bdrv_qed_co_create(BlockdevCreateOptions *opts, >> l1_size = header.cluster_size * header.table_size; >> >> /* File must start empty and grow, check truncate is supported */ > I would update the above comment, with something like > > "QED format requires the underlying file to have the exact expected length, > which is 0 on creation" > Or something similar. I’ll change it to: The QED format associates file length with allocation status, so a new file (which is empty) must have a length of 0. Hope that’s OK. :-) Max
© 2016 - 2025 Red Hat, Inc.