Re: [Qemu-block] [PATCH v8] qemu-img: add the 'dd' subcommand

[Max Reitz]

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