| Message ID | 20160815121149.21464-1-fullmanet@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
On Tue, Aug 16, 2016 at 12:09:06PM +0100, Stefan Hajnoczi wrote: > On Mon, Aug 15, 2016 at 02:11:49PM +0200, Reda Sallahi wrote: > > + 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 I used img_open() here (or img_open_opts()/img_open_file() for that matter) it would have written on stderr that the file couldn't be opened here even though in this case it's not error if we can create the output image and open it later on.
On Fri, Aug 19, 2016 at 09:06:08PM +0200, Reda Sallahi wrote: > On Tue, Aug 16, 2016 at 12:09:06PM +0100, Stefan Hajnoczi wrote: > > On Mon, Aug 15, 2016 at 02:11:49PM +0200, Reda Sallahi wrote: > > > + 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 I used img_open() here (or img_open_opts()/img_open_file() for that > matter) it would have written on stderr that the file couldn't be opened > here even though in this case it's not error if we can create the output > image and open it later on. Then img_open() should be extended so the caller can suppress the error message or gets a Error object instead of output to stderr. Stefan
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); 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)) { + 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); + } + if (!(dd.flags & C_SKIP) || (in.offset <= INT64_MAX / in.bsz && + size >= in.offset * in.bsz)) { + if (!(dd.conv & C_NOTRUNC) || + blk2sz < size - in.offset * in.bsz) { + blk_truncate(blk2, size - in.offset * in.bsz); + } + } } if (dd.flags & C_SKIP && (in.offset > INT64_MAX / in.bsz || @@ -4141,6 +4187,7 @@ static int img_dd(int argc, char **argv) out: g_free(arg); qemu_opts_del(opts); + qemu_opts_del(qopts); qemu_opts_free(create_opts); blk_unref(blk1); blk_unref(blk2);
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(-)