From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52326) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bZcF7-0005Mf-2d for qemu-devel@nongnu.org; Tue, 16 Aug 2016 07:09:14 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bZcF3-0000mx-KK for qemu-devel@nongnu.org; Tue, 16 Aug 2016 07:09:12 -0400 Date: Tue, 16 Aug 2016 12:09:06 +0100 From: Stefan Hajnoczi Message-ID: <20160816110906.GC8157@stefanha-x1.localdomain> References: <20160815121149.21464-1-fullmanet@gmail.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="DIOMP1UsTsWJauNi" Content-Disposition: inline In-Reply-To: <20160815121149.21464-1-fullmanet@gmail.com> Subject: Re: [Qemu-devel] [PATCH v3] qemu-img: change opening method for the output in dd List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Reda Sallahi Cc: qemu-devel@nongnu.org, Kevin Wolf , Fam Zheng , qemu-block@nongnu.org, Max Reitz , Stefan Hajnoczi --DIOMP1UsTsWJauNi Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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 alrea= dy > created. With this patch we try to open first the output image and resize= it > if necessary. >=20 > We also make it mandatory to specify conv=3Dnotrunc when the file already > exists. >=20 > Signed-off-by: Reda Sallahi > --- > Depends on: > [PATCH v3] qemu-img: add conv=3Dnotrunc option to dd >=20 > Changes from v2: > * Remove redundant code > Changes from v1: > * add --image-opts handling >=20 > qemu-img.c | 149 ++++++++++++++++++++++++++++++++++++++++---------------= ------ > 1 file changed, 98 insertions(+), 51 deletions(-) >=20 > 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 =3D NULL, *proto_drv =3D NULL; > BlockBackend *blk1 =3D NULL, *blk2 =3D NULL; > - QemuOpts *opts =3D NULL; > + QDict *qoptions =3D NULL; > + QemuOpts *opts =3D NULL, *qopts =3D NULL; > QemuOptsList *create_opts =3D NULL; > Error *local_err =3D NULL; > bool image_opts =3D false; > @@ -4030,36 +4031,6 @@ static int img_dd(int argc, char **argv) > goto out; > } > =20 > - drv =3D bdrv_find_format(out_fmt); > - if (!drv) { > - error_report("Unknown file format"); > - ret =3D -1; > - goto out; > - } > - proto_drv =3D bdrv_find_protocol(out.filename, true, &local_err); > - > - if (!proto_drv) { > - error_report_err(local_err); > - ret =3D -1; > - goto out; > - } > - if (!drv->create_opts) { > - error_report("Format driver '%s' does not support image creation= ", > - drv->format_name); > - ret =3D -1; > - goto out; > - } > - if (!proto_drv->create_opts) { > - error_report("Protocol driver '%s' does not support image creati= on", > - proto_drv->format_name); > - ret =3D -1; > - goto out; > - } > - create_opts =3D qemu_opts_append(create_opts, drv->create_opts); > - create_opts =3D qemu_opts_append(create_opts, proto_drv->create_opts= ); > - > - opts =3D qemu_opts_create(create_opts, NULL, 0, &error_abort); > - > size =3D 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 =3D 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 =3D qemu_opts_parse_noisily(qemu_find_opts("source"), > + out.filename, true); > + if (!opts) { > + ret =3D -1; > + goto out; > + } > + qoptions =3D qemu_opts_to_qdict(qopts, NULL); > } else { > - qemu_opt_set_number(opts, BLOCK_OPT_SIZE, > - size - in.bsz * in.offset, &error_abort); > + qoptions =3D qdict_new(); > + qdict_put(qoptions, "driver", qstring_from_str(out_fmt)); > } > =20 > - ret =3D 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 =3D -1; > - goto out; > - } > - > - blk2 =3D img_open(image_opts, out.filename, out_fmt, BDRV_O_RDWR, > - false, false); > + blk2 =3D 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()? > =20 > if (!blk2) { > - ret =3D -1; > - goto out; > + drv =3D bdrv_find_format(out_fmt); > + if (!drv) { > + error_report("Unknown file format"); > + ret =3D -1; > + goto out; > + } > + proto_drv =3D bdrv_find_protocol(out.filename, true, &local_err); > + > + if (!proto_drv) { > + error_report_err(local_err); > + ret =3D -1; > + goto out; > + } > + if (!drv->create_opts) { > + error_report("Format driver '%s' does not support image crea= tion", > + drv->format_name); > + ret =3D -1; > + goto out; > + } > + if (!proto_drv->create_opts) { > + error_report("Protocol driver '%s' does not support image cr= eation", > + proto_drv->format_name); > + ret =3D -1; > + goto out; > + } > + create_opts =3D qemu_opts_append(create_opts, drv->create_opts); > + create_opts =3D qemu_opts_append(create_opts, proto_drv->create_= opts); > + > + opts =3D qemu_opts_create(create_opts, NULL, 0, &error_abort); > + > + /* Overflow means the specified offset is beyond input image's s= ize */ > + 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 =3D 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 =3D -1; > + goto out; > + } > + blk2 =3D img_open(image_opts, out.filename, out_fmt, BDRV_O_RDWR, > + false, false); > + if (!blk2) { > + ret =3D -1; > + goto out; > + } > + } else { > + int64_t blk2sz =3D 0; > + > + if (!(dd.conv & C_NOTRUNC)) { > + /* We make conv=3Dnotrunc 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=3Dnotrunc not specified"); > + ret =3D -1; > + goto out; > + } > + > + blk2sz =3D blk_getlength(blk2); > + if (blk2sz < 0) { > + error_report("Failed to get size for '%s'", in.filename); > + ret =3D -1; > + goto out; > + } > + > + if (!(dd.conv & C_NOTRUNC)) { > + blk_truncate(blk2, 0); > + } This is dead code since the check above requires conv=3Dnotrunc. (If it's dead code, it can't be tested and shouldn't be included.) > + if (!(dd.flags & C_SKIP) || (in.offset <=3D INT64_MAX / in.bsz && > + size >=3D 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 =3D size - in.offset * in.bsz; That way the calculation doesn't need to be duplicated several times in the code. --DIOMP1UsTsWJauNi Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQEcBAEBAgAGBQJXsvRSAAoJEJykq7OBq3PIHAQH/jyhIUntsToH7CpfvnLzm/Aq u0q3sRiPk8so5wM+YHWB4o36Ok5Ef7C80aHBRW+VU0qPUtVM51O6QF6dYHj+soDe Rw14kojtG0kmfjIE4pTodOFTk5XirIA8in3X/xeI3JyW8+0vvogsjG66VRlAV7rC jO81YhsAIC02iIrjenhCbxxXjXnTqzQlEKwqUzjKCpGemv7RM4g5FjOj7SSoxHs/ JFo02dQLpNvHo9/pgmb6SXutsY3QpZO63yZ5aBZo4vD5/+N3QLv7ugfKNpSMkWbK tFQjzwaMOxOtbUu5OfI16kz7kI85E2rxrMEvV0vaX1iC1/5sZNpANb8U2lHCr6w= =WK++ -----END PGP SIGNATURE----- --DIOMP1UsTsWJauNi--