diff mbox

[v6] qemu-img: add the 'dd' subcommand

Message ID 20160725055842.3836-1-fullmanet@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Reda Sallahi July 25, 2016, 5:58 a.m. UTC
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 <fullmanet@gmail.com>
---
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                       | 363 ++++++++++++++++++++++++++++++++++++++-
 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, 644 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

Comments

Fam Zheng July 27, 2016, 7:48 a.m. UTC | #1
On Mon, 07/25 07:58, 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 <fullmanet@gmail.com>

Reviewed-by: Fam Zheng <famz@redhat.com>
Stefan Hajnoczi July 28, 2016, 9:19 a.m. UTC | #2
On Mon, Jul 25, 2016 at 6:58 AM, Reda Sallahi <fullmanet@gmail.com> 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 <fullmanet@gmail.com>
> ---
> 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                       | 363 ++++++++++++++++++++++++++++++++++++++-
>  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, 644 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

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Stefan Hajnoczi Aug. 8, 2016, 9:35 a.m. UTC | #3
On Mon, Jul 25, 2016 at 6:58 AM, Reda Sallahi <fullmanet@gmail.com> wrote:
> This patch adds a basic dd subcommand analogous to dd(1) to qemu-img.

Ping?

Reda has additional patches on top of this one.  It would be good to
get this into the block-next branch so his later work can be merged.

> 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 <fullmanet@gmail.com>
> ---
> 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                       | 363 ++++++++++++++++++++++++++++++++++++++-
>  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, 644 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
>
> diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
> index 7e95b2d..03bdd7a 100644
> --- a/qemu-img-cmds.hx
> +++ b/qemu-img-cmds.hx
> @@ -45,6 +45,12 @@ STEXI
>  @item convert [--object @var{objectdef}] [--image-opts] [-c] [-p] [-q] [-n] [-f @var{fmt}] [-t @var{cache}] [-T @var{src_cache}] [-O @var{output_fmt}] [-o @var{options}] [-s @var{snapshot_id_or_name}] [-l @var{snapshot_param}] [-S @var{sparse_size}] @var{filename} [@var{filename2} [...]] @var{output_filename}
>  ETEXI
>
> +DEF("dd", img_dd,
> +    "dd [--image-opts] [-f fmt] [-O output_fmt] [bs=block_size] [count=blocks] if=input of=output")
> +STEXI
> +@item dd [--image-opts] [-f @var{fmt}] [-O @var{output_fmt}] [bs=@var{block_size}] [count=@var{blocks}] if=@var{input} of=@var{output}
> +ETEXI
> +
>  DEF("info", img_info,
>      "info [--object objectdef] [--image-opts] [-f fmt] [--output=ofmt] [--backing-chain] filename")
>  STEXI
> diff --git a/qemu-img.c b/qemu-img.c
> index 2e40e1f..498626b 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -166,7 +166,14 @@ static void QEMU_NORETURN help(void)
>             "Parameters to compare subcommand:\n"
>             "  '-f' first image format\n"
>             "  '-F' second image format\n"
> -           "  '-s' run in Strict mode - fail on different image size or sector allocation\n";
> +           "  '-s' run in Strict mode - fail on different image size or sector allocation\n"
> +           "\n"
> +           "Parameters to dd subcommand:\n"
> +           "  'bs=BYTES' read and write up to BYTES bytes at a time "
> +           "(default: 512)\n"
> +           "  'count=N' copy only N input blocks\n"
> +           "  'if=FILE' read from FILE\n"
> +           "  'of=FILE' write to FILE\n";
>
>      printf("%s\nSupported formats:", help_msg);
>      bdrv_iterate_format(format_print, NULL);
> @@ -3794,6 +3801,360 @@ out:
>      return 0;
>  }
>
> +#define C_BS      01
> +#define C_COUNT   02
> +#define C_IF      04
> +#define C_OF      010
> +
> +struct DdInfo {
> +    unsigned int flags;
> +    size_t count;
> +};
> +
> +struct DdIo {
> +    size_t bsz;    /* Block size */
> +    char *filename;
> +    uint8_t *buf;
> +};
> +
> +struct DdOpts {
> +    const char *name;
> +    int (*f)(const char *, struct DdIo *, struct DdIo *, struct DdInfo *);
> +    unsigned int flag;
> +};
> +
> +/*
> + * get_size() was needed for the size syntax dd(1) supports which is
> + * different from qemu_strtosz_suffix()
> + *
> + */
> +static size_t get_size(const char *str)
> +{
> +    /* XXX: handle {k,m,g}B notations */
> +    unsigned long num;
> +    size_t res = 0;
> +    const char *buf;
> +    int ret;
> +
> +    errno = 0;
> +    if (strchr(str, '-')) {
> +        error_report("invalid number: '%s'", str);
> +        errno = EINVAL;
> +        return res;
> +    }
> +    ret = qemu_strtoul(str, &buf, 0, &num);
> +
> +    if (ret < 0) {
> +        error_report("invalid number: '%s'", str);
> +        return res;
> +    }
> +
> +    switch (*buf) {
> +    case '\0':
> +    case 'c':
> +        res = num;
> +        break;
> +    case 'w':
> +        res = num * 2;
> +        break;
> +    case 'b':
> +        res = num * 512;
> +        break;
> +    case 'K':
> +        res = num * 1024;
> +        break;
> +    case 'M':
> +        res = num * 1024 * 1024;
> +        break;
> +    case 'G':
> +        res = num * 1024 * 1024 * 1024;
> +        break;
> +    case 'T':
> +        res = num * 1024 * 1024 * 1024 * 1024;
> +        break;
> +    case 'P':
> +        res = num * 1024 * 1024 * 1024 * 1024 * 1024;
> +        break;
> +    case 'E':
> +        res = num * 1024 * 1024 * 1024 * 1024 * 1024 * 1024;
> +        break;
> +    case 'Z':
> +        res = num * 1024 * 1024 * 1024 * 1024 * 1024 * 1024 * 1024;
> +        break;
> +    case 'Y':
> +        res = num * 1024 * 1024 * 1024 * 1024 * 1024 * 1024 * 1024 * 1024;
> +        break;
> +    default:
> +        error_report("invalid number: '%s'", str);
> +        errno = EINVAL;
> +    }
> +
> +    return res;
> +}
> +
> +static int img_dd_bs(const char *arg,
> +                     struct DdIo *in, struct DdIo *out,
> +                     struct DdInfo *dd)
> +{
> +    in->bsz = out->bsz = get_size(arg);
> +
> +    if (in->bsz == 0 && (errno == EINVAL || errno == ERANGE)) {
> +        return 1;
> +    }
> +    if (in->bsz == 0) {
> +        error_report("invalid number: '%s'", arg);
> +        return 1;
> +    }
> +
> +    return 0;
> +}
> +
> +static int img_dd_count(const char *arg,
> +                        struct DdIo *in, struct DdIo *out,
> +                        struct DdInfo *dd)
> +{
> +    dd->count = get_size(arg);
> +
> +    if (dd->count == 0 && (errno == EINVAL || errno == ERANGE)) {
> +        return 1;
> +    }
> +
> +    return 0;
> +}
> +
> +static int img_dd_if(const char *arg,
> +                     struct DdIo *in, struct DdIo *out,
> +                     struct DdInfo *dd)
> +{
> +    in->filename = g_strdup(arg);
> +
> +    return 0;
> +}
> +
> +static int img_dd_of(const char *arg,
> +                     struct DdIo *in, struct DdIo *out,
> +                     struct DdInfo *dd)
> +{
> +    out->filename = g_strdup(arg);
> +
> +    return 0;
> +}
> +
> +static int img_dd(int argc, char **argv)
> +{
> +    int ret = 0;
> +    char *arg = NULL;
> +    char *tmp;
> +    BlockDriver *drv = NULL, *proto_drv = NULL;
> +    BlockBackend *blk1 = NULL, *blk2 = NULL;
> +    QemuOpts *opts = NULL;
> +    QemuOptsList *create_opts = NULL;
> +    Error *local_err = NULL;
> +    bool image_opts = false;
> +    int c;
> +    const char *out_fmt = "raw";
> +    const char *fmt = NULL;
> +    int64_t size = 0;
> +    int64_t block_count = 0, incount = 0, outcount = 0;
> +    struct DdInfo dd = {
> +        .flags = 0,
> +        .count = 0,
> +    };
> +    struct DdIo in = {
> +        .bsz = 512, /* Block size is by default 512 bytes */
> +        .filename = NULL,
> +        .buf = NULL
> +    };
> +    struct DdIo out = {
> +        .bsz = 512,
> +        .filename = NULL,
> +        .buf = NULL
> +    };
> +
> +    const struct DdOpts options[] = {
> +        { "bs", img_dd_bs, C_BS },
> +        { "count", img_dd_count, C_COUNT },
> +        { "if", img_dd_if, C_IF },
> +        { "of", img_dd_of, C_OF },
> +        { NULL, NULL, 0 }
> +    };
> +    const struct option long_options[] = {
> +        { "help", no_argument, 0, 'h'},
> +        { "image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
> +        { 0, 0, 0, 0 }
> +    };
> +
> +    while ((c = getopt_long(argc, argv, "hf:O:", long_options, NULL))) {
> +        if (c == EOF) {
> +            break;
> +        }
> +        switch (c) {
> +        case 'O':
> +            out_fmt = optarg;
> +            break;
> +        case 'f':
> +            fmt = optarg;
> +            break;
> +        case '?':
> +            error_report("Try 'qemu-img --help' for more information.");
> +            ret = -1;
> +            goto out;
> +            break;
> +        case 'h':
> +            help();
> +            break;
> +        case OPTION_IMAGE_OPTS:
> +            image_opts = true;
> +            break;
> +        }
> +    }
> +
> +    for (int i = optind; i < argc; i++) {
> +        int j;
> +        arg = g_strdup(argv[i]);
> +
> +        tmp = strchr(arg, '=');
> +        if (tmp == NULL) {
> +            error_report("unrecognized operand %s", arg);
> +            ret = -1;
> +            goto out;
> +        }
> +
> +        *tmp++ = '\0';
> +
> +        for (j = 0; options[j].name != NULL; j++) {
> +            if (!strcmp(arg, options[j].name)) {
> +                break;
> +            }
> +        }
> +        if (options[j].name == NULL) {
> +            error_report("unrecognized operand %s", arg);
> +            ret = -1;
> +            goto out;
> +        }
> +
> +        if (options[j].f(tmp, &in, &out, &dd) != 0) {
> +            ret = -1;
> +            goto out;
> +        }
> +        dd.flags |= options[j].flag;
> +        g_free(arg);
> +        arg = NULL;
> +    }
> +
> +    if (!(dd.flags & C_IF && dd.flags & C_OF)) {
> +        error_report("Must specify both input and output files");
> +        ret = -1;
> +        goto out;
> +    }
> +    blk1 = img_open(image_opts, in.filename, fmt, 0, false, true);
> +
> +    if (!blk1) {
> +        ret = -1;
> +        goto out;
> +    }
> +
> +    drv = bdrv_find_format(out_fmt);
> +    if (!drv) {
> +        error_report("Unknown file format");
> +        ret = -1;
> +        goto out;
> +    }
> +    proto_drv = bdrv_find_protocol(out.filename, true, &local_err);
> +
> +    if (!proto_drv) {
> +        error_report_err(local_err);
> +        ret = -1;
> +        goto out;
> +    }
> +    if (!drv->create_opts) {
> +        error_report("Format driver '%s' does not support image creation",
> +                     drv->format_name);
> +        ret = -1;
> +        goto out;
> +    }
> +    if (!proto_drv->create_opts) {
> +        error_report("Protocol driver '%s' does not support image creation",
> +                     proto_drv->format_name);
> +        ret = -1;
> +        goto out;
> +    }
> +    create_opts = qemu_opts_append(create_opts, drv->create_opts);
> +    create_opts = qemu_opts_append(create_opts, proto_drv->create_opts);
> +
> +    opts = qemu_opts_create(create_opts, NULL, 0, &error_abort);
> +
> +    size =  blk_getlength(blk1);
> +
> +    if (dd.flags & C_COUNT && dd.count * in.bsz < size) {
> +        size = dd.count * in.bsz;
> +    }
> +
> +    qemu_opt_set_number(opts, BLOCK_OPT_SIZE, size, &error_abort);
> +
> +    ret = bdrv_create(drv, out.filename, opts, &local_err);
> +    if (ret < 0) {
> +        error_reportf_err(local_err,
> +                          "%s: error while creating output image: ",
> +                          out.filename);
> +        ret = -1;
> +        goto out;
> +    }
> +
> +    blk2 = img_open(image_opts, out.filename, out_fmt, BDRV_O_RDWR,
> +                    false, true);
> +
> +    if (!blk2) {
> +        ret = -1;
> +        goto out;
> +    }
> +
> +    in.buf = g_new(uint8_t, in.bsz);
> +
> +    for (; incount < size; block_count++) {
> +        int in_ret, out_ret;
> +
> +        if (in.bsz + incount > size) {
> +            in_ret = blk_pread(blk1, incount, in.buf, size - incount);
> +        } else {
> +            in_ret = blk_pread(blk1, incount, in.buf, in.bsz);
> +        }
> +        if (in_ret < 0) {
> +            error_report("error while reading from input image file: %s",
> +                         strerror(-in_ret));
> +            ret = -1;
> +            goto out;
> +        }
> +        incount += in_ret;
> +
> +        out_ret = blk_pwrite(blk2, outcount, in.buf, in_ret, 0);
> +
> +        if (out_ret < 0) {
> +            error_report("error while writing to output image file: %s",
> +                         strerror(-out_ret));
> +            ret = -1;
> +            goto out;
> +        }
> +        outcount += out_ret;
> +    }
> +
> +out:
> +    g_free(arg);
> +    qemu_opts_del(opts);
> +    qemu_opts_free(create_opts);
> +    blk_unref(blk1);
> +    blk_unref(blk2);
> +    g_free(in.filename);
> +    g_free(out.filename);
> +    g_free(in.buf);
> +    g_free(out.buf);
> +
> +    if (ret) {
> +        return 1;
> +    }
> +    return 0;
> +}
> +
>
>  static const img_cmd_t img_cmds[] = {
>  #define DEF(option, callback, arg_string)        \
> diff --git a/qemu-img.texi b/qemu-img.texi
> index 449a19c..880293a 100644
> --- a/qemu-img.texi
> +++ b/qemu-img.texi
> @@ -139,6 +139,20 @@ Parameters to convert subcommand:
>  Skip the creation of the target volume
>  @end table
>
> +Parameters to dd subcommand:
> +
> +@table @option
> +
> +@item bs=@var{block_size}
> +defines the block size
> +@item count=@var{blocks}
> +sets the number of input blocks to copy
> +@item if=@var{input}
> +sets the input file
> +@item of=@var{output}
> +sets the output file
> +@end table
> +
>  Command description:
>
>  @table @option
> @@ -310,6 +324,17 @@ skipped. This is useful for formats such as @code{rbd} if the target
>  volume has already been created with site specific options that cannot
>  be supplied through qemu-img.
>
> +@item dd [-f @var{fmt}] [-O @var{output_fmt}] [bs=@var{block_size}] [count=@var{blocks}] if=@var{input} of=@var{output}
> +
> +Dd copies from @var{input} file to @var{output} file converting it from
> +@var{fmt} format to @var{output_fmt} format.
> +
> +The data is by default read and written using blocks of 512 bytes but can be
> +modified by specifying @var{block_size}. If count=@var{blocks} is specified
> +dd will stop reading input after reading @var{blocks} input blocks.
> +
> +The size syntax is similar to dd(1)'s size syntax.
> +
>  @item info [-f @var{fmt}] [--output=@var{ofmt}] [--backing-chain] @var{filename}
>
>  Give information about the disk image @var{filename}. Use it in
> 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 @@
> +#! /bin/bash
> +#
> +# qemu-img dd test
> +#
> +# Copyright (C) 2016 Reda Sallahi
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +#
> +
> +owner=fullmanet@gmail.com
> +
> +seq="$(basename $0)"
> +echo "QA output created by $seq"
> +
> +here="$PWD"
> +status=1
> +
> +_cleanup()
> +{
> +    _cleanup_test_img
> +    rm -f "$TEST_IMG.out"
> +}
> +trap "_cleanup; exit \$status" 0 1 2 3 15
> +
> +. ./common.rc
> +. ./common.filter
> +. ./common.pattern
> +
> +_supported_fmt generic
> +_supported_proto file
> +_supported_os Linux
> +
> +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 =="
> +
> +$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
> +
> +echo
> +echo "== Compare the images with qemu-img compare =="
> +
> +$QEMU_IMG compare "$TEST_IMG" "$TEST_IMG.out"
> +
> +echo
> +echo "*** done"
> +rm -f "$seq.full"
> +status=0
> diff --git a/tests/qemu-iotests/158.out b/tests/qemu-iotests/158.out
> new file mode 100644
> index 0000000..58bfd85
> --- /dev/null
> +++ b/tests/qemu-iotests/158.out
> @@ -0,0 +1,15 @@
> +QA output created by 158
> +
> +== Creating image ==
> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576
> +No errors were found on the image.
> +wrote 1048576/1048576 bytes at offset 0
> +1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +
> +== Converting the image with dd ==
> +No errors were found on the image.
> +
> +== Compare the images with qemu-img compare ==
> +Images are identical.
> +
> +*** done
> 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 @@
> +#! /bin/bash
> +#
> +# qemu-img dd test with different block sizes
> +#
> +# Copyright (C) 2016 Reda Sallahi
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +#
> +
> +owner=fullmanet@gmail.com
> +
> +seq="$(basename $0)"
> +echo "QA output created by $seq"
> +
> +here="$PWD"
> +status=1
> +
> +_cleanup()
> +{
> +    _cleanup_test_img
> +    rm -f "$TEST_IMG.out"
> +}
> +trap "_cleanup; exit \$status" 0 1 2 3 15
> +
> +. ./common.rc
> +. ./common.filter
> +. ./common.pattern
> +
> +_supported_fmt generic
> +_supported_proto file
> +_supported_os Linux
> +
> +TEST_SIZES="5 512 1024 1999 1K 64K 1M"
> +
> +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
> +    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/159.out b/tests/qemu-iotests/159.out
> new file mode 100644
> index 0000000..b86b63a
> --- /dev/null
> +++ b/tests/qemu-iotests/159.out
> @@ -0,0 +1,87 @@
> +QA output created by 159
> +
> +== Creating image ==
> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576
> +No errors were found on the image.
> +wrote 1048576/1048576 bytes at offset 0
> +1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +
> +== Converting the image with dd with a block size of 5 ==
> +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 1048576/1048576 bytes at offset 0
> +1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +
> +== Converting the image with dd with a block size of 512 ==
> +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 1048576/1048576 bytes at offset 0
> +1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +
> +== Converting the image with dd with a block size of 1024 ==
> +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 1048576/1048576 bytes at offset 0
> +1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +
> +== Converting the image with dd with a block size of 1999 ==
> +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 1048576/1048576 bytes at offset 0
> +1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +
> +== Converting the image with dd with a block size of 1K ==
> +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 1048576/1048576 bytes at offset 0
> +1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +
> +== Converting the image with dd with a block size of 64K ==
> +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 1048576/1048576 bytes at offset 0
> +1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +
> +== Converting the image with dd with a block size of 1M ==
> +No errors were found on the image.
> +
> +== Compare the images with qemu-img compare ==
> +Images are identical.
> +
> +*** done
> diff --git a/tests/qemu-iotests/common.filter b/tests/qemu-iotests/common.filter
> index 7853dbb..79d322a 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'
> +}
> +
>  # Removes \r from messages
>  _filter_win32()
>  {
> diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
> index 3a3973e..ec712bc 100644
> --- a/tests/qemu-iotests/group
> +++ b/tests/qemu-iotests/group
> @@ -157,3 +157,5 @@
>  155 rw auto
>  156 rw auto quick
>  157 auto
> +158 rw auto quick
> +159 rw auto quick
> --
> 2.9.0
>
>
Max Reitz Aug. 8, 2016, 1:36 p.m. UTC | #4
On 25.07.2016 07:58, 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 <fullmanet@gmail.com>
> ---
> 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                       | 363 ++++++++++++++++++++++++++++++++++++++-
>  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, 644 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
> 
> diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
> index 7e95b2d..03bdd7a 100644
> --- a/qemu-img-cmds.hx
> +++ b/qemu-img-cmds.hx
> @@ -45,6 +45,12 @@ STEXI
>  @item convert [--object @var{objectdef}] [--image-opts] [-c] [-p] [-q] [-n] [-f @var{fmt}] [-t @var{cache}] [-T @var{src_cache}] [-O @var{output_fmt}] [-o @var{options}] [-s @var{snapshot_id_or_name}] [-l @var{snapshot_param}] [-S @var{sparse_size}] @var{filename} [@var{filename2} [...]] @var{output_filename}
>  ETEXI
>  
> +DEF("dd", img_dd,
> +    "dd [--image-opts] [-f fmt] [-O output_fmt] [bs=block_size] [count=blocks] if=input of=output")
> +STEXI
> +@item dd [--image-opts] [-f @var{fmt}] [-O @var{output_fmt}] [bs=@var{block_size}] [count=@var{blocks}] if=@var{input} of=@var{output}
> +ETEXI
> +
>  DEF("info", img_info,
>      "info [--object objectdef] [--image-opts] [-f fmt] [--output=ofmt] [--backing-chain] filename")
>  STEXI
> diff --git a/qemu-img.c b/qemu-img.c
> index 2e40e1f..498626b 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -166,7 +166,14 @@ static void QEMU_NORETURN help(void)
>             "Parameters to compare subcommand:\n"
>             "  '-f' first image format\n"
>             "  '-F' second image format\n"
> -           "  '-s' run in Strict mode - fail on different image size or sector allocation\n";
> +           "  '-s' run in Strict mode - fail on different image size or sector allocation\n"
> +           "\n"
> +           "Parameters to dd subcommand:\n"
> +           "  'bs=BYTES' read and write up to BYTES bytes at a time "
> +           "(default: 512)\n"
> +           "  'count=N' copy only N input blocks\n"
> +           "  'if=FILE' read from FILE\n"
> +           "  'of=FILE' write to FILE\n";
>  
>      printf("%s\nSupported formats:", help_msg);
>      bdrv_iterate_format(format_print, NULL);
> @@ -3794,6 +3801,360 @@ out:
>      return 0;
>  }
>  
> +#define C_BS      01
> +#define C_COUNT   02
> +#define C_IF      04
> +#define C_OF      010

Not sure why you used octal here, but since two people already gave
their R-b, I guess it at least has some novelty value. ;-)

Also, I think I'd personally use an enum for this, but that is very much
personal taste.

> +
> +struct DdInfo {
> +    unsigned int flags;
> +    size_t count;

I'd strongly suggest using uint64_t because the width of size_t depends
on the platform.

> +};
> +
> +struct DdIo {
> +    size_t bsz;    /* Block size */

I wouldn't use size_t here because of the above reason. However, I would
not use uint64_t here, since you will be passing this value to g_new()
and also to blk_pread(), so I'd use the type of the latter's parameter,
which is int.

> +    char *filename;
> +    uint8_t *buf;
> +};
> +
> +struct DdOpts {
> +    const char *name;
> +    int (*f)(const char *, struct DdIo *, struct DdIo *, struct DdInfo *);
> +    unsigned int flag;
> +};
> +
> +/*
> + * get_size() was needed for the size syntax dd(1) supports which is
> + * different from qemu_strtosz_suffix()
> + *
> + */
> +static size_t get_size(const char *str)

This function should return a uint64_t, otherwise you won't be able to
go beyond 4 GB on 32 bit machines.

Also, I'd strongly suggest giving this function an Error **errp
parameter, because I really don't like doing internal error handling
using errno.

> +{
> +    /* XXX: handle {k,m,g}B notations */
> +    unsigned long num;
> +    size_t res = 0;
> +    const char *buf;
> +    int ret;
> +
> +    errno = 0;
> +    if (strchr(str, '-')) {
> +        error_report("invalid number: '%s'", str);
> +        errno = EINVAL;
> +        return res;
> +    }

Not sure what this test is for since qemu_strtoul() will reject negative
numbers anyway, as far as I understand.

> +    ret = qemu_strtoul(str, &buf, 0, &num);
> +
> +    if (ret < 0) {
> +        error_report("invalid number: '%s'", str);

With an Error ** parameter, you'd use the following here:

error_setg_errno(errp, -ret, "invalid number '%s', str);

> +        return res;
> +    }
> +
> +    switch (*buf) {
> +    case '\0':
> +    case 'c':
> +        res = num;
> +        break;
> +    case 'w':
> +        res = num * 2;
> +        break;
> +    case 'b':
> +        res = num * 512;
> +        break;
> +    case 'K':
> +        res = num * 1024;
> +        break;
> +    case 'M':
> +        res = num * 1024 * 1024;
> +        break;
> +    case 'G':
> +        res = num * 1024 * 1024 * 1024;
> +        break;
> +    case 'T':
> +        res = num * 1024 * 1024 * 1024 * 1024;

Starting from here, these multiplications will overflow on systems where
sizeof(size_t) <= 4, i.e. on 32 bit platforms. I suggest using uint64_t
for num and qemu_strtoull() above.

> +        break;
> +    case 'P':
> +        res = num * 1024 * 1024 * 1024 * 1024 * 1024;
> +        break;
> +    case 'E':
> +        res = num * 1024 * 1024 * 1024 * 1024 * 1024 * 1024;
> +        break;
> +    case 'Z':
> +        res = num * 1024 * 1024 * 1024 * 1024 * 1024 * 1024 * 1024;
> +        break;
> +    case 'Y':
> +        res = num * 1024 * 1024 * 1024 * 1024 * 1024 * 1024 * 1024 * 1024;
> +        break;
> +    default:
> +        error_report("invalid number: '%s'", str);
> +        errno = EINVAL;
> +    }
> +
> +    return res;
> +}
> +
> +static int img_dd_bs(const char *arg,
> +                     struct DdIo *in, struct DdIo *out,
> +                     struct DdInfo *dd)

If you give get_size() an Error ** parameter, I'd suggest giving these
error evaluation functions one, too. You could then pass errors from
get_size() on to the caller.

> +{
> +    in->bsz = out->bsz = get_size(arg);

If you make get_size() return a uint64_t and bsz an int, you will have
to check for overflows here.

> +
> +    if (in->bsz == 0 && (errno == EINVAL || errno == ERANGE)) {
> +        return 1;
> +    }
> +    if (in->bsz == 0) {
> +        error_report("invalid number: '%s'", arg);

It's not an invalid number, it's just an invalid block size. In my
understanding, those are two different things.

> +        return 1;
> +    }
> +
> +    return 0;
> +}
> +
> +static int img_dd_count(const char *arg,
> +                        struct DdIo *in, struct DdIo *out,
> +                        struct DdInfo *dd)
> +{
> +    dd->count = get_size(arg);
> +
> +    if (dd->count == 0 && (errno == EINVAL || errno == ERANGE)) {
> +        return 1;
> +    }
> +
> +    return 0;
> +}
> +
> +static int img_dd_if(const char *arg,
> +                     struct DdIo *in, struct DdIo *out,
> +                     struct DdInfo *dd)
> +{
> +    in->filename = g_strdup(arg);
> +
> +    return 0;
> +}
> +
> +static int img_dd_of(const char *arg,
> +                     struct DdIo *in, struct DdIo *out,
> +                     struct DdInfo *dd)
> +{
> +    out->filename = g_strdup(arg);
> +
> +    return 0;
> +}
> +
> +static int img_dd(int argc, char **argv)
> +{
> +    int ret = 0;
> +    char *arg = NULL;
> +    char *tmp;
> +    BlockDriver *drv = NULL, *proto_drv = NULL;
> +    BlockBackend *blk1 = NULL, *blk2 = NULL;
> +    QemuOpts *opts = NULL;
> +    QemuOptsList *create_opts = NULL;
> +    Error *local_err = NULL;
> +    bool image_opts = false;
> +    int c;
> +    const char *out_fmt = "raw";
> +    const char *fmt = NULL;
> +    int64_t size = 0;
> +    int64_t block_count = 0, incount = 0, outcount = 0;
> +    struct DdInfo dd = {
> +        .flags = 0,
> +        .count = 0,
> +    };
> +    struct DdIo in = {
> +        .bsz = 512, /* Block size is by default 512 bytes */
> +        .filename = NULL,
> +        .buf = NULL
> +    };
> +    struct DdIo out = {
> +        .bsz = 512,
> +        .filename = NULL,
> +        .buf = NULL
> +    };
> +
> +    const struct DdOpts options[] = {
> +        { "bs", img_dd_bs, C_BS },
> +        { "count", img_dd_count, C_COUNT },
> +        { "if", img_dd_if, C_IF },
> +        { "of", img_dd_of, C_OF },
> +        { NULL, NULL, 0 }
> +    };
> +    const struct option long_options[] = {
> +        { "help", no_argument, 0, 'h'},
> +        { "image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
> +        { 0, 0, 0, 0 }
> +    };
> +
> +    while ((c = getopt_long(argc, argv, "hf:O:", long_options, NULL))) {
> +        if (c == EOF) {
> +            break;
> +        }
> +        switch (c) {
> +        case 'O':
> +            out_fmt = optarg;
> +            break;
> +        case 'f':
> +            fmt = optarg;
> +            break;
> +        case '?':
> +            error_report("Try 'qemu-img --help' for more information.");
> +            ret = -1;
> +            goto out;
> +            break;

The break is rather unnecessary with the goto in front of it.

> +        case 'h':
> +            help();
> +            break;
> +        case OPTION_IMAGE_OPTS:
> +            image_opts = true;
> +            break;
> +        }
> +    }
> +
> +    for (int i = optind; i < argc; i++) {
> +        int j;
> +        arg = g_strdup(argv[i]);
> +
> +        tmp = strchr(arg, '=');

FYI, strtok() is a neat function for exactly this. I'm not saying you
need to use it, though.

> +        if (tmp == NULL) {
> +            error_report("unrecognized operand %s", arg);
> +            ret = -1;
> +            goto out;
> +        }
> +
> +        *tmp++ = '\0';
> +
> +        for (j = 0; options[j].name != NULL; j++) {
> +            if (!strcmp(arg, options[j].name)) {
> +                break;
> +            }
> +        }
> +        if (options[j].name == NULL) {
> +            error_report("unrecognized operand %s", arg);
> +            ret = -1;
> +            goto out;
> +        }
> +
> +        if (options[j].f(tmp, &in, &out, &dd) != 0) {
> +            ret = -1;
> +            goto out;
> +        }
> +        dd.flags |= options[j].flag;
> +        g_free(arg);
> +        arg = NULL;
> +    }
> +
> +    if (!(dd.flags & C_IF && dd.flags & C_OF)) {
> +        error_report("Must specify both input and output files");
> +        ret = -1;
> +        goto out;
> +    }
> +    blk1 = img_open(image_opts, in.filename, fmt, 0, false, true);

I'm not quite sure why you're passing true for quiet here.

> +
> +    if (!blk1) {
> +        ret = -1;
> +        goto out;
> +    }
> +
> +    drv = bdrv_find_format(out_fmt);
> +    if (!drv) {
> +        error_report("Unknown file format");
> +        ret = -1;
> +        goto out;
> +    }
> +    proto_drv = bdrv_find_protocol(out.filename, true, &local_err);
> +
> +    if (!proto_drv) {
> +        error_report_err(local_err);
> +        ret = -1;
> +        goto out;
> +    }
> +    if (!drv->create_opts) {
> +        error_report("Format driver '%s' does not support image creation",
> +                     drv->format_name);
> +        ret = -1;
> +        goto out;
> +    }
> +    if (!proto_drv->create_opts) {
> +        error_report("Protocol driver '%s' does not support image creation",
> +                     proto_drv->format_name);
> +        ret = -1;
> +        goto out;
> +    }
> +    create_opts = qemu_opts_append(create_opts, drv->create_opts);
> +    create_opts = qemu_opts_append(create_opts, proto_drv->create_opts);
> +
> +    opts = qemu_opts_create(create_opts, NULL, 0, &error_abort);
> +
> +    size =  blk_getlength(blk1);

There's a superfluous space after the equal sign. Also, blk_getlength()
may fail, you should catch that case.

> +
> +    if (dd.flags & C_COUNT && dd.count * in.bsz < size) {
> +        size = dd.count * in.bsz;
> +    }
> +
> +    qemu_opt_set_number(opts, BLOCK_OPT_SIZE, size, &error_abort);
> +
> +    ret = bdrv_create(drv, out.filename, opts, &local_err);
> +    if (ret < 0) {
> +        error_reportf_err(local_err,
> +                          "%s: error while creating output image: ",
> +                          out.filename);
> +        ret = -1;
> +        goto out;
> +    }
> +
> +    blk2 = img_open(image_opts, out.filename, out_fmt, BDRV_O_RDWR,
> +                    false, true);

Again, not sure why quiet is true.

> +
> +    if (!blk2) {
> +        ret = -1;
> +        goto out;
> +    }
> +
> +    in.buf = g_new(uint8_t, in.bsz);
> +
> +    for (; incount < size; block_count++) {

Please do not initialize incount above but here. Having to jump more
than a hundred lines up to find out what a variable is set to makes code
very hard to read.

Also, I'd rename incount to in_offset or something, because "incount" to
me sounds like it counts a number of blocks, whereas it actually counts
a number of bytes, independently of the block size.

> +        int in_ret, out_ret;
> +
> +        if (in.bsz + incount > size) {

Not wrong, but I'd turn the summands around, i.e. "incount + in.bsz" (or
"in_offset + in.bsz"). That makes more sense intuitively, since you're
adding the new block you want to read to the offset where you already are.

Max

> +            in_ret = blk_pread(blk1, incount, in.buf, size - incount);
> +        } else {
> +            in_ret = blk_pread(blk1, incount, in.buf, in.bsz);
> +        }
> +        if (in_ret < 0) {
> +            error_report("error while reading from input image file: %s",
> +                         strerror(-in_ret));
> +            ret = -1;
> +            goto out;
> +        }
> +        incount += in_ret;
> +
> +        out_ret = blk_pwrite(blk2, outcount, in.buf, in_ret, 0);
> +
> +        if (out_ret < 0) {
> +            error_report("error while writing to output image file: %s",
> +                         strerror(-out_ret));
> +            ret = -1;
> +            goto out;
> +        }
> +        outcount += out_ret;
> +    }
> +
> +out:
> +    g_free(arg);
> +    qemu_opts_del(opts);
> +    qemu_opts_free(create_opts);
> +    blk_unref(blk1);
> +    blk_unref(blk2);
> +    g_free(in.filename);
> +    g_free(out.filename);
> +    g_free(in.buf);
> +    g_free(out.buf);
> +
> +    if (ret) {
> +        return 1;
> +    }
> +    return 0;
> +}
> +
>  
>  static const img_cmd_t img_cmds[] = {
>  #define DEF(option, callback, arg_string)        \
Kevin Wolf Aug. 9, 2016, 9:16 a.m. UTC | #5
Am 25.07.2016 um 07:58 hat Reda Sallahi geschrieben:
> 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 <fullmanet@gmail.com>

> +/*
> + * get_size() was needed for the size syntax dd(1) supports which is
> + * different from qemu_strtosz_suffix()
> + *
> + */

(Excess empty line in comment)

Is it really a good idea to stay consistent with dd when this makes the
subcommand inconsistent with all other size specifications in qemu? If I
understand correctly, the only difference is that some suffixes wouldn't
be supported, so you would get an error message rather than surprising
behaviour. I would consider that fine and probably preferrable to adding
another size parser. Nobody uses 'c', 'w' or 'b' anyway.

Kevin
Reda Sallahi Aug. 9, 2016, 6:39 p.m. UTC | #6
Hi Max,
Thanks for the review!

On 8/8/16, Max Reitz <mreitz@redhat.com> wrote:
> On 25.07.2016 07:58, Reda Sallahi wrote:
>> +    if (in->bsz == 0) {
>> +        error_report("invalid number: '%s'", arg);
>
> It's not an invalid number, it's just an invalid block size. In my
> understanding, those are two different things.

I was trying to have the same error message as dd(1) for this.

>> +        tmp = strchr(arg, '=');
>
> FYI, strtok() is a neat function for exactly this. I'm not saying you
> need to use it, though.

For something as simple I would rather avoid using strtok().

>> +    for (; incount < size; block_count++) {
>
> Please do not initialize incount above but here. Having to jump more
> than a hundred lines up to find out what a variable is set to makes code
> very hard to read.
>
> Also, I'd rename incount to in_offset or something, because "incount" to
> me sounds like it counts a number of blocks, whereas it actually counts
> a number of bytes, independently of the block size.
>

There is already in.offset ([PATCH v4] qemu-img: add skip option to dd)
so it will just be confusing but if you have a better name I will be glad to
change it.
Max Reitz Aug. 9, 2016, 8:43 p.m. UTC | #7
On 09.08.2016 20:39, Reda Sallahi wrote:
> Hi Max,
> Thanks for the review!
> 
> On 8/8/16, Max Reitz <mreitz@redhat.com> wrote:
>> On 25.07.2016 07:58, Reda Sallahi wrote:
>>> +    if (in->bsz == 0) {
>>> +        error_report("invalid number: '%s'", arg);
>>
>> It's not an invalid number, it's just an invalid block size. In my
>> understanding, those are two different things.
> 
> I was trying to have the same error message as dd(1) for this.

OK. I hope nobody is really relying on dd emitting exactly this error
message, though. :-)

>>> +        tmp = strchr(arg, '=');
>>
>> FYI, strtok() is a neat function for exactly this. I'm not saying you
>> need to use it, though.
> 
> For something as simple I would rather avoid using strtok().

Yep, that's fine.

>>> +    for (; incount < size; block_count++) {
>>
>> Please do not initialize incount above but here. Having to jump more
>> than a hundred lines up to find out what a variable is set to makes code
>> very hard to read.
>>
>> Also, I'd rename incount to in_offset or something, because "incount" to
>> me sounds like it counts a number of blocks, whereas it actually counts
>> a number of bytes, independently of the block size.
>>
> 
> There is already in.offset ([PATCH v4] qemu-img: add skip option to dd)
> so it will just be confusing but if you have a better name I will be glad to
> change it.

Hm. How about "in_position"?

Max
diff mbox

Patch

diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
index 7e95b2d..03bdd7a 100644
--- a/qemu-img-cmds.hx
+++ b/qemu-img-cmds.hx
@@ -45,6 +45,12 @@  STEXI
 @item convert [--object @var{objectdef}] [--image-opts] [-c] [-p] [-q] [-n] [-f @var{fmt}] [-t @var{cache}] [-T @var{src_cache}] [-O @var{output_fmt}] [-o @var{options}] [-s @var{snapshot_id_or_name}] [-l @var{snapshot_param}] [-S @var{sparse_size}] @var{filename} [@var{filename2} [...]] @var{output_filename}
 ETEXI
 
+DEF("dd", img_dd,
+    "dd [--image-opts] [-f fmt] [-O output_fmt] [bs=block_size] [count=blocks] if=input of=output")
+STEXI
+@item dd [--image-opts] [-f @var{fmt}] [-O @var{output_fmt}] [bs=@var{block_size}] [count=@var{blocks}] if=@var{input} of=@var{output}
+ETEXI
+
 DEF("info", img_info,
     "info [--object objectdef] [--image-opts] [-f fmt] [--output=ofmt] [--backing-chain] filename")
 STEXI
diff --git a/qemu-img.c b/qemu-img.c
index 2e40e1f..498626b 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -166,7 +166,14 @@  static void QEMU_NORETURN help(void)
            "Parameters to compare subcommand:\n"
            "  '-f' first image format\n"
            "  '-F' second image format\n"
-           "  '-s' run in Strict mode - fail on different image size or sector allocation\n";
+           "  '-s' run in Strict mode - fail on different image size or sector allocation\n"
+           "\n"
+           "Parameters to dd subcommand:\n"
+           "  'bs=BYTES' read and write up to BYTES bytes at a time "
+           "(default: 512)\n"
+           "  'count=N' copy only N input blocks\n"
+           "  'if=FILE' read from FILE\n"
+           "  'of=FILE' write to FILE\n";
 
     printf("%s\nSupported formats:", help_msg);
     bdrv_iterate_format(format_print, NULL);
@@ -3794,6 +3801,360 @@  out:
     return 0;
 }
 
+#define C_BS      01
+#define C_COUNT   02
+#define C_IF      04
+#define C_OF      010
+
+struct DdInfo {
+    unsigned int flags;
+    size_t count;
+};
+
+struct DdIo {
+    size_t bsz;    /* Block size */
+    char *filename;
+    uint8_t *buf;
+};
+
+struct DdOpts {
+    const char *name;
+    int (*f)(const char *, struct DdIo *, struct DdIo *, struct DdInfo *);
+    unsigned int flag;
+};
+
+/*
+ * get_size() was needed for the size syntax dd(1) supports which is
+ * different from qemu_strtosz_suffix()
+ *
+ */
+static size_t get_size(const char *str)
+{
+    /* XXX: handle {k,m,g}B notations */
+    unsigned long num;
+    size_t res = 0;
+    const char *buf;
+    int ret;
+
+    errno = 0;
+    if (strchr(str, '-')) {
+        error_report("invalid number: '%s'", str);
+        errno = EINVAL;
+        return res;
+    }
+    ret = qemu_strtoul(str, &buf, 0, &num);
+
+    if (ret < 0) {
+        error_report("invalid number: '%s'", str);
+        return res;
+    }
+
+    switch (*buf) {
+    case '\0':
+    case 'c':
+        res = num;
+        break;
+    case 'w':
+        res = num * 2;
+        break;
+    case 'b':
+        res = num * 512;
+        break;
+    case 'K':
+        res = num * 1024;
+        break;
+    case 'M':
+        res = num * 1024 * 1024;
+        break;
+    case 'G':
+        res = num * 1024 * 1024 * 1024;
+        break;
+    case 'T':
+        res = num * 1024 * 1024 * 1024 * 1024;
+        break;
+    case 'P':
+        res = num * 1024 * 1024 * 1024 * 1024 * 1024;
+        break;
+    case 'E':
+        res = num * 1024 * 1024 * 1024 * 1024 * 1024 * 1024;
+        break;
+    case 'Z':
+        res = num * 1024 * 1024 * 1024 * 1024 * 1024 * 1024 * 1024;
+        break;
+    case 'Y':
+        res = num * 1024 * 1024 * 1024 * 1024 * 1024 * 1024 * 1024 * 1024;
+        break;
+    default:
+        error_report("invalid number: '%s'", str);
+        errno = EINVAL;
+    }
+
+    return res;
+}
+
+static int img_dd_bs(const char *arg,
+                     struct DdIo *in, struct DdIo *out,
+                     struct DdInfo *dd)
+{
+    in->bsz = out->bsz = get_size(arg);
+
+    if (in->bsz == 0 && (errno == EINVAL || errno == ERANGE)) {
+        return 1;
+    }
+    if (in->bsz == 0) {
+        error_report("invalid number: '%s'", arg);
+        return 1;
+    }
+
+    return 0;
+}
+
+static int img_dd_count(const char *arg,
+                        struct DdIo *in, struct DdIo *out,
+                        struct DdInfo *dd)
+{
+    dd->count = get_size(arg);
+
+    if (dd->count == 0 && (errno == EINVAL || errno == ERANGE)) {
+        return 1;
+    }
+
+    return 0;
+}
+
+static int img_dd_if(const char *arg,
+                     struct DdIo *in, struct DdIo *out,
+                     struct DdInfo *dd)
+{
+    in->filename = g_strdup(arg);
+
+    return 0;
+}
+
+static int img_dd_of(const char *arg,
+                     struct DdIo *in, struct DdIo *out,
+                     struct DdInfo *dd)
+{
+    out->filename = g_strdup(arg);
+
+    return 0;
+}
+
+static int img_dd(int argc, char **argv)
+{
+    int ret = 0;
+    char *arg = NULL;
+    char *tmp;
+    BlockDriver *drv = NULL, *proto_drv = NULL;
+    BlockBackend *blk1 = NULL, *blk2 = NULL;
+    QemuOpts *opts = NULL;
+    QemuOptsList *create_opts = NULL;
+    Error *local_err = NULL;
+    bool image_opts = false;
+    int c;
+    const char *out_fmt = "raw";
+    const char *fmt = NULL;
+    int64_t size = 0;
+    int64_t block_count = 0, incount = 0, outcount = 0;
+    struct DdInfo dd = {
+        .flags = 0,
+        .count = 0,
+    };
+    struct DdIo in = {
+        .bsz = 512, /* Block size is by default 512 bytes */
+        .filename = NULL,
+        .buf = NULL
+    };
+    struct DdIo out = {
+        .bsz = 512,
+        .filename = NULL,
+        .buf = NULL
+    };
+
+    const struct DdOpts options[] = {
+        { "bs", img_dd_bs, C_BS },
+        { "count", img_dd_count, C_COUNT },
+        { "if", img_dd_if, C_IF },
+        { "of", img_dd_of, C_OF },
+        { NULL, NULL, 0 }
+    };
+    const struct option long_options[] = {
+        { "help", no_argument, 0, 'h'},
+        { "image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
+        { 0, 0, 0, 0 }
+    };
+
+    while ((c = getopt_long(argc, argv, "hf:O:", long_options, NULL))) {
+        if (c == EOF) {
+            break;
+        }
+        switch (c) {
+        case 'O':
+            out_fmt = optarg;
+            break;
+        case 'f':
+            fmt = optarg;
+            break;
+        case '?':
+            error_report("Try 'qemu-img --help' for more information.");
+            ret = -1;
+            goto out;
+            break;
+        case 'h':
+            help();
+            break;
+        case OPTION_IMAGE_OPTS:
+            image_opts = true;
+            break;
+        }
+    }
+
+    for (int i = optind; i < argc; i++) {
+        int j;
+        arg = g_strdup(argv[i]);
+
+        tmp = strchr(arg, '=');
+        if (tmp == NULL) {
+            error_report("unrecognized operand %s", arg);
+            ret = -1;
+            goto out;
+        }
+
+        *tmp++ = '\0';
+
+        for (j = 0; options[j].name != NULL; j++) {
+            if (!strcmp(arg, options[j].name)) {
+                break;
+            }
+        }
+        if (options[j].name == NULL) {
+            error_report("unrecognized operand %s", arg);
+            ret = -1;
+            goto out;
+        }
+
+        if (options[j].f(tmp, &in, &out, &dd) != 0) {
+            ret = -1;
+            goto out;
+        }
+        dd.flags |= options[j].flag;
+        g_free(arg);
+        arg = NULL;
+    }
+
+    if (!(dd.flags & C_IF && dd.flags & C_OF)) {
+        error_report("Must specify both input and output files");
+        ret = -1;
+        goto out;
+    }
+    blk1 = img_open(image_opts, in.filename, fmt, 0, false, true);
+
+    if (!blk1) {
+        ret = -1;
+        goto out;
+    }
+
+    drv = bdrv_find_format(out_fmt);
+    if (!drv) {
+        error_report("Unknown file format");
+        ret = -1;
+        goto out;
+    }
+    proto_drv = bdrv_find_protocol(out.filename, true, &local_err);
+
+    if (!proto_drv) {
+        error_report_err(local_err);
+        ret = -1;
+        goto out;
+    }
+    if (!drv->create_opts) {
+        error_report("Format driver '%s' does not support image creation",
+                     drv->format_name);
+        ret = -1;
+        goto out;
+    }
+    if (!proto_drv->create_opts) {
+        error_report("Protocol driver '%s' does not support image creation",
+                     proto_drv->format_name);
+        ret = -1;
+        goto out;
+    }
+    create_opts = qemu_opts_append(create_opts, drv->create_opts);
+    create_opts = qemu_opts_append(create_opts, proto_drv->create_opts);
+
+    opts = qemu_opts_create(create_opts, NULL, 0, &error_abort);
+
+    size =  blk_getlength(blk1);
+
+    if (dd.flags & C_COUNT && dd.count * in.bsz < size) {
+        size = dd.count * in.bsz;
+    }
+
+    qemu_opt_set_number(opts, BLOCK_OPT_SIZE, size, &error_abort);
+
+    ret = bdrv_create(drv, out.filename, opts, &local_err);
+    if (ret < 0) {
+        error_reportf_err(local_err,
+                          "%s: error while creating output image: ",
+                          out.filename);
+        ret = -1;
+        goto out;
+    }
+
+    blk2 = img_open(image_opts, out.filename, out_fmt, BDRV_O_RDWR,
+                    false, true);
+
+    if (!blk2) {
+        ret = -1;
+        goto out;
+    }
+
+    in.buf = g_new(uint8_t, in.bsz);
+
+    for (; incount < size; block_count++) {
+        int in_ret, out_ret;
+
+        if (in.bsz + incount > size) {
+            in_ret = blk_pread(blk1, incount, in.buf, size - incount);
+        } else {
+            in_ret = blk_pread(blk1, incount, in.buf, in.bsz);
+        }
+        if (in_ret < 0) {
+            error_report("error while reading from input image file: %s",
+                         strerror(-in_ret));
+            ret = -1;
+            goto out;
+        }
+        incount += in_ret;
+
+        out_ret = blk_pwrite(blk2, outcount, in.buf, in_ret, 0);
+
+        if (out_ret < 0) {
+            error_report("error while writing to output image file: %s",
+                         strerror(-out_ret));
+            ret = -1;
+            goto out;
+        }
+        outcount += out_ret;
+    }
+
+out:
+    g_free(arg);
+    qemu_opts_del(opts);
+    qemu_opts_free(create_opts);
+    blk_unref(blk1);
+    blk_unref(blk2);
+    g_free(in.filename);
+    g_free(out.filename);
+    g_free(in.buf);
+    g_free(out.buf);
+
+    if (ret) {
+        return 1;
+    }
+    return 0;
+}
+
 
 static const img_cmd_t img_cmds[] = {
 #define DEF(option, callback, arg_string)        \
diff --git a/qemu-img.texi b/qemu-img.texi
index 449a19c..880293a 100644
--- a/qemu-img.texi
+++ b/qemu-img.texi
@@ -139,6 +139,20 @@  Parameters to convert subcommand:
 Skip the creation of the target volume
 @end table
 
+Parameters to dd subcommand:
+
+@table @option
+
+@item bs=@var{block_size}
+defines the block size
+@item count=@var{blocks}
+sets the number of input blocks to copy
+@item if=@var{input}
+sets the input file
+@item of=@var{output}
+sets the output file
+@end table
+
 Command description:
 
 @table @option
@@ -310,6 +324,17 @@  skipped. This is useful for formats such as @code{rbd} if the target
 volume has already been created with site specific options that cannot
 be supplied through qemu-img.
 
+@item dd [-f @var{fmt}] [-O @var{output_fmt}] [bs=@var{block_size}] [count=@var{blocks}] if=@var{input} of=@var{output}
+
+Dd copies from @var{input} file to @var{output} file converting it from
+@var{fmt} format to @var{output_fmt} format.
+
+The data is by default read and written using blocks of 512 bytes but can be
+modified by specifying @var{block_size}. If count=@var{blocks} is specified
+dd will stop reading input after reading @var{blocks} input blocks.
+
+The size syntax is similar to dd(1)'s size syntax.
+
 @item info [-f @var{fmt}] [--output=@var{ofmt}] [--backing-chain] @var{filename}
 
 Give information about the disk image @var{filename}. Use it in
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 @@ 
+#! /bin/bash
+#
+# qemu-img dd test
+#
+# Copyright (C) 2016 Reda Sallahi
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+owner=fullmanet@gmail.com
+
+seq="$(basename $0)"
+echo "QA output created by $seq"
+
+here="$PWD"
+status=1
+
+_cleanup()
+{
+    _cleanup_test_img
+    rm -f "$TEST_IMG.out"
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+. ./common.rc
+. ./common.filter
+. ./common.pattern
+
+_supported_fmt generic
+_supported_proto file
+_supported_os Linux
+
+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 =="
+
+$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
+
+echo
+echo "== Compare the images with qemu-img compare =="
+
+$QEMU_IMG compare "$TEST_IMG" "$TEST_IMG.out"
+
+echo
+echo "*** done"
+rm -f "$seq.full"
+status=0
diff --git a/tests/qemu-iotests/158.out b/tests/qemu-iotests/158.out
new file mode 100644
index 0000000..58bfd85
--- /dev/null
+++ b/tests/qemu-iotests/158.out
@@ -0,0 +1,15 @@ 
+QA output created by 158
+
+== Creating image ==
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576
+No errors were found on the image.
+wrote 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+== Converting the image with dd ==
+No errors were found on the image.
+
+== Compare the images with qemu-img compare ==
+Images are identical.
+
+*** done
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 @@ 
+#! /bin/bash
+#
+# qemu-img dd test with different block sizes
+#
+# Copyright (C) 2016 Reda Sallahi
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+owner=fullmanet@gmail.com
+
+seq="$(basename $0)"
+echo "QA output created by $seq"
+
+here="$PWD"
+status=1
+
+_cleanup()
+{
+    _cleanup_test_img
+    rm -f "$TEST_IMG.out"
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+. ./common.rc
+. ./common.filter
+. ./common.pattern
+
+_supported_fmt generic
+_supported_proto file
+_supported_os Linux
+
+TEST_SIZES="5 512 1024 1999 1K 64K 1M"
+
+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
+    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/159.out b/tests/qemu-iotests/159.out
new file mode 100644
index 0000000..b86b63a
--- /dev/null
+++ b/tests/qemu-iotests/159.out
@@ -0,0 +1,87 @@ 
+QA output created by 159
+
+== Creating image ==
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576
+No errors were found on the image.
+wrote 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+== Converting the image with dd with a block size of 5 ==
+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 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+== Converting the image with dd with a block size of 512 ==
+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 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+== Converting the image with dd with a block size of 1024 ==
+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 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+== Converting the image with dd with a block size of 1999 ==
+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 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+== Converting the image with dd with a block size of 1K ==
+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 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+== Converting the image with dd with a block size of 64K ==
+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 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+== Converting the image with dd with a block size of 1M ==
+No errors were found on the image.
+
+== Compare the images with qemu-img compare ==
+Images are identical.
+
+*** done
diff --git a/tests/qemu-iotests/common.filter b/tests/qemu-iotests/common.filter
index 7853dbb..79d322a 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'
+}
+
 # Removes \r from messages
 _filter_win32()
 {
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 3a3973e..ec712bc 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -157,3 +157,5 @@ 
 155 rw auto
 156 rw auto quick
 157 auto
+158 rw auto quick
+159 rw auto quick