[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v8] qemu-img: add the 'dd' subcommand
From: |
Max Reitz |
Subject: |
Re: [Qemu-devel] [PATCH v8] qemu-img: add the 'dd' subcommand |
Date: |
Wed, 10 Aug 2016 01:10:03 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 |
On 09.08.2016 23:20, Reda Sallahi wrote:
> This patch adds a basic dd subcommand analogous to dd(1) to qemu-img.
>
> For the start, this implements the bs, if, of and count options and requires
> both if and of to be specified (no stdin/stdout if not specified) and doesn't
> support tty, pipes, etc.
>
> The image format must be specified with -O for the output if the raw format
> is not the intended one.
>
> Two tests are added to test qemu-img dd.
>
> Signed-off-by: Reda Sallahi <address@hidden>
> ---
> Changes from v7:
> * Remove a C99-style for loop.
> Changes from v6:
> * Remove get_size() to use qemu_strtosz_suffix() instead.
> * Type changes for some fields in DdIo and DdInfo.
> Changes from v5:
> * Add dd sections on qemu-img.texi.
> Changes from v4:
> * Fix the exit status.
> Changes from v3:
> * Delete an unused (so far) field in DdIo.
> Changes from v2:
> * Add copyright headers to new files.
> Changes from v1:
> * Removal of dead code.
> * Fix a memory leak.
> * Complete the cleanup function in the test cases.
>
> qemu-img-cmds.hx | 6 +
> qemu-img.c | 302
> ++++++++++++++++++++++++++++++++++++++-
> qemu-img.texi | 25 ++++
> tests/qemu-iotests/158 | 68 +++++++++
> tests/qemu-iotests/158.out | 15 ++
> tests/qemu-iotests/159 | 70 +++++++++
> tests/qemu-iotests/159.out | 87 +++++++++++
> tests/qemu-iotests/common.filter | 9 ++
> tests/qemu-iotests/group | 2 +
> 9 files changed, 583 insertions(+), 1 deletion(-)
> create mode 100755 tests/qemu-iotests/158
> create mode 100644 tests/qemu-iotests/158.out
> create mode 100755 tests/qemu-iotests/159
> create mode 100644 tests/qemu-iotests/159.out
Looks good overall, just some minor comments below (and maybe you still
want to replace {in,out}count by {in,out}_position anyway).
[...]
> diff --git a/qemu-img.c b/qemu-img.c
> index d2865a5..10aaf0e 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
[...]
> @@ -3794,6 +3801,299 @@ out:
[...]
> + size = blk_getlength(blk1);
> + if (size < 0) {
> + error_report("Failed to get size for '%s'", in.filename);
> + ret = -1;
> + goto out;
> + }
> +
> + if (dd.flags & C_COUNT && dd.count * in.bsz < size) {
dd.count * in.bsz can overflow, there should probably be a check for
that before this point.
> + size = dd.count * in.bsz;
> + }
[...]
> diff --git a/tests/qemu-iotests/158 b/tests/qemu-iotests/158
> new file mode 100755
> index 0000000..48972c8
> --- /dev/null
> +++ b/tests/qemu-iotests/158
> @@ -0,0 +1,68 @@
[...]
> +echo
> +echo "== Converting the image with dd =="
> +
> +$QEMU_IMG dd if="$TEST_IMG" of="$TEST_IMG.out" -O "$IMGFMT"
> +$QEMU_IMG check "$TEST_IMG.out" -f "$IMGFMT" 2>&1 | _filter_testdir | \
> + _filter_qemu_img_check
A simpler way of doing this would be:
TEST_IMG="$TEST_IMG.out" _check_test_img
[...]
> diff --git a/tests/qemu-iotests/159 b/tests/qemu-iotests/159
> new file mode 100755
> index 0000000..e55d942
> --- /dev/null
> +++ b/tests/qemu-iotests/159
> @@ -0,0 +1,70 @@
[...]
> +for bs in $TEST_SIZES; do
> + echo
> + echo "== Creating image =="
> +
> + size=1M
> + _make_test_img $size
> + _check_test_img
> + $QEMU_IO -c "write -P 0xa 0 $size" "$TEST_IMG" | _filter_qemu_io
> +
> + echo
> + echo "== Converting the image with dd with a block size of $bs =="
> +
> + $QEMU_IMG dd if="$TEST_IMG" of="$TEST_IMG.out" bs=$bs -O "$IMGFMT"
> + $QEMU_IMG check "$TEST_IMG.out" -f "$IMGFMT" 2>&1 | _filter_testdir | \
> + _filter_qemu_img_check
Again, TEST_IMG="$TEST_IMG.out" _check_test_img would be simpler.
> + echo
> + echo "== Compare the images with qemu-img compare =="
> +
> + $QEMU_IMG compare "$TEST_IMG" "$TEST_IMG.out"
> +done
> +
> +echo
> +echo "*** done"
> +rm -f "$seq.full"
> +status=0
[...]
> diff --git a/tests/qemu-iotests/common.filter
> b/tests/qemu-iotests/common.filter
> index 3ab6e4d..cef5222 100644
> --- a/tests/qemu-iotests/common.filter
> +++ b/tests/qemu-iotests/common.filter
> @@ -44,6 +44,15 @@ _filter_imgfmt()
> sed -e "s#$IMGFMT#IMGFMT#g"
> }
>
> +# replace error message when the format is not supported and delete
> +# the output lines after the first one
> +_filter_qemu_img_check()
> +{
> + sed -e '/allocated.*fragmented.*compressed clusters/d' \
> + -e 's/qemu-img: This image format does not support checks/No errors
> were found on the image./' \
> + -e '/Image end offset: [0-9]\+/d'
> +}
> +
If you use the TEST_IMG="$TEST_IMG.out" _check_test_img shorthand I've
proposed above, then this function will no longer be necessary.
If you don't, then please make use of this function in _check_test_img.
That function contains exactly the same code (probably not by chance),
so you should replace that code by a call to this function.
Max
signature.asc
Description: OpenPGP digital signature