Re: [Qemu-devel] [PATCH v3] qemu-img: change opening method for the output in dd

[Stefan Hajnoczi <stefanha@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 8011 bytes --]

On Mon, Aug 15, 2016 at 02:11:49PM +0200, Reda Sallahi wrote:
> dd was creating an output image regardless of whether there was one already
> created. With this patch we try to open first the output image and resize it
> if necessary.
> 
> We also make it mandatory to specify conv=notrunc when the file already
> exists.
> 
> Signed-off-by: Reda Sallahi <fullmanet@gmail.com>
> ---
> Depends on:
> [PATCH v3] qemu-img: add conv=notrunc option to dd
> 
> Changes from v2:
> * Remove redundant code
> Changes from v1:
> * add --image-opts handling
> 
>  qemu-img.c | 149 ++++++++++++++++++++++++++++++++++++++++---------------------
>  1 file changed, 98 insertions(+), 51 deletions(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index d08905b..8af6dd9 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -3919,7 +3919,8 @@ static int img_dd(int argc, char **argv)
>      char *tmp;
>      BlockDriver *drv = NULL, *proto_drv = NULL;
>      BlockBackend *blk1 = NULL, *blk2 = NULL;
> -    QemuOpts *opts = NULL;
> +    QDict *qoptions = NULL;
> +    QemuOpts *opts = NULL, *qopts = NULL;
>      QemuOptsList *create_opts = NULL;
>      Error *local_err = NULL;
>      bool image_opts = false;
> @@ -4030,36 +4031,6 @@ static int img_dd(int argc, char **argv)
>          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 (size < 0) {
>          error_report("Failed to get size for '%s'", in.filename);
> @@ -4071,31 +4042,106 @@ static int img_dd(int argc, char **argv)
>          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);
> +    if (image_opts) {
> +        qopts = qemu_opts_parse_noisily(qemu_find_opts("source"),
> +                                        out.filename, true);
> +        if (!opts) {
> +            ret = -1;
> +            goto out;
> +        }
> +        qoptions = qemu_opts_to_qdict(qopts, NULL);
>      } else {
> -        qemu_opt_set_number(opts, BLOCK_OPT_SIZE,
> -                            size - in.bsz * in.offset, &error_abort);
> +        qoptions = qdict_new();
> +        qdict_put(qoptions, "driver", qstring_from_str(out_fmt));
>      }
>  
> -    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, false);
> +    blk2 = blk_new_open(image_opts ? NULL : out.filename,
> +                        NULL, qoptions, BDRV_O_RDWR, NULL);

This code duplicates a subset of img_open().  Why can't you use
img_open() or at least img_open_opts()/img_open_file()?

>  
>      if (!blk2) {
> -        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);
> +
> +        /* Overflow means the specified offset is beyond input image's size */
> +        if (dd.flags & C_SKIP && (in.offset > INT64_MAX / in.bsz ||
> +                                  size < in.offset * in.bsz)) {

Please drop dd.flags & C_SKIP, it makes the expression more complicated
but doesn't add anything.  We only need to look at in.offset.

> +            qemu_opt_set_number(opts, BLOCK_OPT_SIZE,
> +                                0, &error_abort);
> +        } else {
> +            qemu_opt_set_number(opts, BLOCK_OPT_SIZE,
> +                                size - in.offset * in.bsz, &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, false);
> +        if (!blk2) {
> +            ret = -1;
> +            goto out;
> +        }
> +    } else {
> +        int64_t blk2sz = 0;
> +
> +        if (!(dd.conv & C_NOTRUNC)) {
> +            /* We make conv=notrunc mandatory for the moment to avoid
> +               accidental destruction of the output image. Needs to be
> +               changed when a better solution is found */
> +            error_report("conv=notrunc not specified");
> +            ret = -1;
> +            goto out;
> +        }
> +
> +        blk2sz = blk_getlength(blk2);
> +        if (blk2sz < 0) {
> +            error_report("Failed to get size for '%s'", in.filename);
> +            ret = -1;
> +            goto out;
> +        }
> +
> +        if (!(dd.conv & C_NOTRUNC)) {
> +            blk_truncate(blk2, 0);
> +        }

This is dead code since the check above requires conv=notrunc.

(If it's dead code, it can't be tested and shouldn't be included.)

> +        if (!(dd.flags & C_SKIP) || (in.offset <= INT64_MAX / in.bsz &&
> +                                     size >= in.offset * in.bsz)) {

C_SKIP check isn't needed.

> +            if (!(dd.conv & C_NOTRUNC) ||
> +                blk2sz < size - in.offset * in.bsz) {
> +                blk_truncate(blk2, size - in.offset * in.bsz);
> +            }

It would be cleaner to calculate the output size upfront:

output_size = size - in.offset * in.bsz;

That way the calculation doesn't need to be duplicated several times in
the code.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]