When both skip= and count= are active, qemu-img dd was not copying
enough data. It didn't help that the code made the same check for
dd.flags & C_SKIP in two separate places. Compute 'size' as the
amount of bytes to be read, and 'end' as the offset to end at,
rather than trying to cram both meanings into a single variable
(which only worked as long as we had at most one of those two
limiting factors to worry about, but not both).
Enhance the test to cover more combinations, and expose the problem.
Signed-off-by: Eric Blake <eblake@redhat.com>
CC: qemu-stable@nongnu.org
---
qemu-img.c | 39 ++++++++++++++++---------------------
tests/qemu-iotests/160 | 9 ++++++---
tests/qemu-iotests/160.out | 48 ++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 70 insertions(+), 26 deletions(-)
diff --git a/qemu-img.c b/qemu-img.c
index 1acddf693c6..d72f0f0ec94 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -4398,7 +4398,7 @@ static int img_dd(int argc, char **argv)
const char *out_fmt = "raw";
const char *fmt = NULL;
int64_t size = 0;
- int64_t block_count = 0, out_pos, in_pos;
+ int64_t block_count = 0, out_pos, in_pos, end;
bool force_share = false;
struct DdInfo dd = {
.flags = 0,
@@ -4559,19 +4559,23 @@ static int img_dd(int argc, char **argv)
goto out;
}
+ /* Overflow means the specified offset is beyond input image's size */
+ if (dd.flags & C_SKIP && (in.offset > INT64_MAX / in.bsz ||
+ size < in.bsz * in.offset)) {
+ size = 0;
+ error_report("%s: cannot skip to specified offset", in.filename);
+ } else {
+ size -= in.offset * in.bsz;
+ in_pos = in.offset * in.bsz;
+ }
+
if (dd.flags & C_COUNT && dd.count <= INT64_MAX / in.bsz &&
dd.count * in.bsz < size) {
size = dd.count * in.bsz;
}
- /* Overflow means the specified offset is beyond input image's size */
- if (dd.flags & C_SKIP && (in.offset > INT64_MAX / in.bsz ||
- size < in.bsz * in.offset)) {
- qemu_opt_set_number(opts, BLOCK_OPT_SIZE, 0, &error_abort);
- } else {
- qemu_opt_set_number(opts, BLOCK_OPT_SIZE,
- size - in.bsz * in.offset, &error_abort);
- }
+ qemu_opt_set_number(opts, BLOCK_OPT_SIZE, size, &error_abort);
+ end = size + in_pos;
ret = bdrv_create(drv, out.filename, opts, &local_err);
if (ret < 0) {
@@ -4595,24 +4599,13 @@ static int img_dd(int argc, char **argv)
goto out;
}
- if (dd.flags & C_SKIP && (in.offset > INT64_MAX / in.bsz ||
- size < in.offset * in.bsz)) {
- /* We give a warning if the skip option is bigger than the input
- * size and create an empty output disk image (i.e. like dd(1)).
- */
- error_report("%s: cannot skip to specified offset", in.filename);
- in_pos = size;
- } else {
- in_pos = in.offset * in.bsz;
- }
-
in.buf = g_new(uint8_t, in.bsz);
- for (out_pos = 0; in_pos < size; block_count++) {
+ for (out_pos = 0; in_pos < end; block_count++) {
int in_ret, out_ret;
- if (in_pos + in.bsz > size) {
- in_ret = blk_pread(blk1, in_pos, in.buf, size - in_pos);
+ if (in_pos + in.bsz > end) {
+ in_ret = blk_pread(blk1, in_pos, in.buf, end - in_pos);
} else {
in_ret = blk_pread(blk1, in_pos, in.buf, in.bsz);
}
diff --git a/tests/qemu-iotests/160 b/tests/qemu-iotests/160
index 5c910e5bfc1..48380a3aafc 100755
--- a/tests/qemu-iotests/160
+++ b/tests/qemu-iotests/160
@@ -44,6 +44,7 @@ _supported_os Linux
TEST_SKIP_BLOCKS="1 2 30 30K"
for skip in $TEST_SKIP_BLOCKS; do
+ for count in '' 'count=1 '; do
echo
echo "== Creating image =="
@@ -53,17 +54,19 @@ for skip in $TEST_SKIP_BLOCKS; do
$QEMU_IO -c "write -P 0xa 24 512k" "$TEST_IMG" | _filter_qemu_io
echo
- echo "== Converting the image with dd with skip=$skip =="
+ echo "== Converting the image with dd with ${count}skip=$skip =="
- $QEMU_IMG dd if="$TEST_IMG" of="$TEST_IMG.out" skip="$skip" -O "$IMGFMT" \
+ $QEMU_IMG dd if="$TEST_IMG" of="$TEST_IMG.out" $count skip="$skip" -O "$IMGFMT" \
2> /dev/null
TEST_IMG="$TEST_IMG.out" _check_test_img
- dd if="$TEST_IMG" of="$TEST_IMG.out.dd" skip="$skip" status=none
+ dd if="$TEST_IMG" of="$TEST_IMG.out.dd" $count skip="$skip" status=none
echo
echo "== Compare the images with qemu-img compare =="
$QEMU_IMG compare "$TEST_IMG.out.dd" "$TEST_IMG.out"
+ rm "$TEST_IMG.out.dd"
+ done
done
echo
diff --git a/tests/qemu-iotests/160.out b/tests/qemu-iotests/160.out
index 9cedc803566..6147a8493d6 100644
--- a/tests/qemu-iotests/160.out
+++ b/tests/qemu-iotests/160.out
@@ -18,6 +18,18 @@ No errors were found on the image.
wrote 524288/524288 bytes at offset 24
512 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+== Converting the image with dd with count=1 skip=1 ==
+No errors were found on the image.
+
+== Compare the images with qemu-img compare ==
+Images are identical.
+
+== Creating image ==
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576
+No errors were found on the image.
+wrote 524288/524288 bytes at offset 24
+512 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
== Converting the image with dd with skip=2 ==
No errors were found on the image.
@@ -30,6 +42,18 @@ No errors were found on the image.
wrote 524288/524288 bytes at offset 24
512 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+== Converting the image with dd with count=1 skip=2 ==
+No errors were found on the image.
+
+== Compare the images with qemu-img compare ==
+Images are identical.
+
+== Creating image ==
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576
+No errors were found on the image.
+wrote 524288/524288 bytes at offset 24
+512 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
== Converting the image with dd with skip=30 ==
No errors were found on the image.
@@ -42,10 +66,34 @@ No errors were found on the image.
wrote 524288/524288 bytes at offset 24
512 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+== Converting the image with dd with count=1 skip=30 ==
+No errors were found on the image.
+
+== Compare the images with qemu-img compare ==
+Images are identical.
+
+== Creating image ==
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576
+No errors were found on the image.
+wrote 524288/524288 bytes at offset 24
+512 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
== Converting the image with dd with skip=30K ==
No errors were found on the image.
== Compare the images with qemu-img compare ==
Images are identical.
+== Creating image ==
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576
+No errors were found on the image.
+wrote 524288/524288 bytes at offset 24
+512 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+== Converting the image with dd with count=1 skip=30K ==
+No errors were found on the image.
+
+== Compare the images with qemu-img compare ==
+Images are identical.
+
*** done
--
2.14.4
On 2018-08-15 04:56, Eric Blake wrote:
> When both skip= and count= are active, qemu-img dd was not copying
> enough data. It didn't help that the code made the same check for
> dd.flags & C_SKIP in two separate places. Compute 'size' as the
> amount of bytes to be read, and 'end' as the offset to end at,
> rather than trying to cram both meanings into a single variable
> (which only worked as long as we had at most one of those two
> limiting factors to worry about, but not both).
>
> Enhance the test to cover more combinations, and expose the problem.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> CC: qemu-stable@nongnu.org
> ---
> qemu-img.c | 39 ++++++++++++++++---------------------
> tests/qemu-iotests/160 | 9 ++++++---
> tests/qemu-iotests/160.out | 48 ++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 70 insertions(+), 26 deletions(-)
>
> diff --git a/qemu-img.c b/qemu-img.c
> index 1acddf693c6..d72f0f0ec94 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
[...]
> @@ -4559,19 +4559,23 @@ static int img_dd(int argc, char **argv)
> goto out;
> }
>
> + /* Overflow means the specified offset is beyond input image's size */
> + if (dd.flags & C_SKIP && (in.offset > INT64_MAX / in.bsz ||
> + size < in.bsz * in.offset)) {
> + size = 0;
> + error_report("%s: cannot skip to specified offset", in.filename);
in_pos should be initialized as well (to "size", I suppose), or my gcc
will continue to complain. :-)
The rest looks good to me.
> + } else {
> + size -= in.offset * in.bsz;
> + in_pos = in.offset * in.bsz;
> + }
> +
[...]
> diff --git a/tests/qemu-iotests/160 b/tests/qemu-iotests/160
> index 5c910e5bfc1..48380a3aafc 100755
> --- a/tests/qemu-iotests/160
> +++ b/tests/qemu-iotests/160
> @@ -44,6 +44,7 @@ _supported_os Linux
> TEST_SKIP_BLOCKS="1 2 30 30K"
>
> for skip in $TEST_SKIP_BLOCKS; do
> + for count in '' 'count=1 '; do
Ah, so this is why we indent everything by four spaces! So you can
squeeze in three more block headers without having to re-indent
everything. I finally see. O:-)
(Not sure why you put a space after the 'count=1', though, but OK)
Max
> echo
> echo "== Creating image =="
>
On 08/15/2018 09:03 PM, Max Reitz wrote:
>> @@ -4559,19 +4559,23 @@ static int img_dd(int argc, char **argv)
>> goto out;
>> }
>>
>> + /* Overflow means the specified offset is beyond input image's size */
>> + if (dd.flags & C_SKIP && (in.offset > INT64_MAX / in.bsz ||
>> + size < in.bsz * in.offset)) {
>> + size = 0;
>> + error_report("%s: cannot skip to specified offset", in.filename);
>
> in_pos should be initialized as well (to "size", I suppose), or my gcc
> will continue to complain. :-)
>
Serves me right for compiling with -g instead of -O2 (gcc only does that
warning on optimized builds, for some weird reason). Will fix in v2.
>> +++ b/tests/qemu-iotests/160
>> @@ -44,6 +44,7 @@ _supported_os Linux
>> TEST_SKIP_BLOCKS="1 2 30 30K"
>>
>> for skip in $TEST_SKIP_BLOCKS; do
>> + for count in '' 'count=1 '; do
>
> Ah, so this is why we indent everything by four spaces! So you can
> squeeze in three more block headers without having to re-indent
> everything. I finally see. O:-)
I'm seriously thinking of reindenting things in this and the next patch,
rather than my initial quick-and-dirty "squeeze it in". 'git diff -w'
is not that hard to use, after all.
>
> (Not sure why you put a space after the 'count=1', though, but OK)
For this line:
+ echo "== Converting the image with dd with ${count}skip=$skip =="
so that when $count is empty, the .out file doesn't end up with a double
space. Okay, I do have some sense of output aesthetics, even if my
re-indentation skills are lacking ;=)
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
On 2018-08-16 04:17, Eric Blake wrote:
> On 08/15/2018 09:03 PM, Max Reitz wrote:
>
>>> @@ -4559,19 +4559,23 @@ static int img_dd(int argc, char **argv)
>>> goto out;
>>> }
>>>
>>> + /* Overflow means the specified offset is beyond input image's
>>> size */
>>> + if (dd.flags & C_SKIP && (in.offset > INT64_MAX / in.bsz ||
>>> + size < in.bsz * in.offset)) {
>>> + size = 0;
>>> + error_report("%s: cannot skip to specified offset",
>>> in.filename);
>>
>> in_pos should be initialized as well (to "size", I suppose), or my gcc
>> will continue to complain. :-)
>>
>
> Serves me right for compiling with -g instead of -O2 (gcc only does that
> warning on optimized builds, for some weird reason). Will fix in v2.
>
>
>>> +++ b/tests/qemu-iotests/160
>>> @@ -44,6 +44,7 @@ _supported_os Linux
>>> TEST_SKIP_BLOCKS="1 2 30 30K"
>>>
>>> for skip in $TEST_SKIP_BLOCKS; do
>>> + for count in '' 'count=1 '; do
>>
>> Ah, so this is why we indent everything by four spaces! So you can
>> squeeze in three more block headers without having to re-indent
>> everything. I finally see. O:-)
>
> I'm seriously thinking of reindenting things in this and the next patch,
> rather than my initial quick-and-dirty "squeeze it in". 'git diff -w'
> is not that hard to use, after all.
>
>>
>> (Not sure why you put a space after the 'count=1', though, but OK)
>
> For this line:
>
> + echo "== Converting the image with dd with ${count}skip=$skip =="
>
> so that when $count is empty, the .out file doesn't end up with a double
> space. Okay, I do have some sense of output aesthetics, even if my
> re-indentation skills are lacking ;=)
Ah, right, I forgot you'd get the double space. That's true.
Max
© 2016 - 2025 Red Hat, Inc.