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 <[email protected]> > --- > 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.
signature.asc
Description: PGP signature
