* [Qemu-devel] [PATCH v3] qemu-img: change opening method for the output in dd
@ 2016-08-15 12:11 Reda Sallahi
2016-08-16 11:09 ` Stefan Hajnoczi
0 siblings, 1 reply; 4+ messages in thread
From: Reda Sallahi @ 2016-08-15 12:11 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-block, Kevin Wolf, Max Reitz, Fam Zheng, Stefan Hajnoczi,
Reda Sallahi
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);
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);
--
2.9.3
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [Qemu-devel] [PATCH v3] qemu-img: change opening method for the output in dd
2016-08-15 12:11 [Qemu-devel] [PATCH v3] qemu-img: change opening method for the output in dd Reda Sallahi
@ 2016-08-16 11:09 ` Stefan Hajnoczi
2016-08-19 19:06 ` Reda Sallahi
0 siblings, 1 reply; 4+ messages in thread
From: Stefan Hajnoczi @ 2016-08-16 11:09 UTC (permalink / raw)
To: Reda Sallahi
Cc: qemu-devel, Kevin Wolf, Fam Zheng, qemu-block, Max Reitz,
Stefan Hajnoczi
[-- 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 --]
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [Qemu-devel] [PATCH v3] qemu-img: change opening method for the output in dd
2016-08-16 11:09 ` Stefan Hajnoczi
@ 2016-08-19 19:06 ` Reda Sallahi
2016-08-22 11:39 ` Stefan Hajnoczi
0 siblings, 1 reply; 4+ messages in thread
From: Reda Sallahi @ 2016-08-19 19:06 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: qemu-devel, Kevin Wolf, Fam Zheng, qemu-block, Max Reitz,
Stefan Hajnoczi
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.
--
Reda
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH v3] qemu-img: change opening method for the output in dd
2016-08-19 19:06 ` Reda Sallahi
@ 2016-08-22 11:39 ` Stefan Hajnoczi
0 siblings, 0 replies; 4+ messages in thread
From: Stefan Hajnoczi @ 2016-08-22 11:39 UTC (permalink / raw)
To: Reda Sallahi
Cc: qemu-devel, Kevin Wolf, Fam Zheng, qemu-block, Max Reitz,
Stefan Hajnoczi
[-- Attachment #1: Type: text/plain, Size: 875 bytes --]
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-08-22 11:39 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-08-15 12:11 [Qemu-devel] [PATCH v3] qemu-img: change opening method for the output in dd Reda Sallahi
2016-08-16 11:09 ` Stefan Hajnoczi
2016-08-19 19:06 ` Reda Sallahi
2016-08-22 11:39 ` Stefan Hajnoczi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).