From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46594) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WsAF5-00017U-Ih for qemu-devel@nongnu.org; Wed, 04 Jun 2014 08:24:39 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WsAEy-00081D-0l for qemu-devel@nongnu.org; Wed, 04 Jun 2014 08:24:31 -0400 Received: from lputeaux-656-01-25-125.w80-12.abo.wanadoo.fr ([80.12.84.125]:45404 helo=paradis.irqsave.net) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WsAEx-000812-N9 for qemu-devel@nongnu.org; Wed, 04 Jun 2014 08:24:23 -0400 Date: Wed, 4 Jun 2014 14:24:21 +0200 From: =?iso-8859-1?Q?Beno=EEt?= Canet Message-ID: <20140604122421.GA30223@irqsave.net> References: <1401882711-30508-1-git-send-email-armbru@redhat.com> <1401882711-30508-11-git-send-email-armbru@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <1401882711-30508-11-git-send-email-armbru@redhat.com> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v4 10/10] block: Avoid bdrv_get_geometry() where errors should be detected List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: kwolf@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com, mreitz@redhat.com The Wednesday 04 Jun 2014 =E0 13:51:51 (+0200), Markus Armbruster wrote : > bdrv_get_geometry() hides errors. Use bdrv_nb_sectors() or > bdrv_getlength() instead where that's obviously inappropriate. >=20 > Signed-off-by: Markus Armbruster > Reviewed-by: Eric Blake > Reviewed-by: Max Reitz > --- > block.c | 11 ++++++++--- > block/qapi.c | 14 +++++++++---- > qemu-img.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++-----= -------- > 3 files changed, 68 insertions(+), 21 deletions(-) >=20 > diff --git a/block.c b/block.c > index cae9085..b92f05f 100644 > --- a/block.c > +++ b/block.c > @@ -5574,7 +5574,7 @@ void bdrv_img_create(const char *filename, const = char *fmt, > if (size && size->value.n =3D=3D -1) { > if (backing_file && backing_file->value.s) { > BlockDriverState *bs; > - uint64_t size; > + int64_t size; > char buf[32]; > int back_flags; > =20 > @@ -5593,8 +5593,13 @@ void bdrv_img_create(const char *filename, const= char *fmt, > local_err =3D NULL; > goto out; > } > - bdrv_get_geometry(bs, &size); > - size *=3D 512; > + size =3D bdrv_getlength(bs); > + if (size < 0) { > + error_setg_errno(errp, -size, "Could not get size of '= %s'", > + backing_file->value.s); > + bdrv_unref(bs); > + goto out; > + } > =20 > snprintf(buf, sizeof(buf), "%" PRId64, size); > set_option_parameter(param, BLOCK_OPT_SIZE, buf); > diff --git a/block/qapi.c b/block/qapi.c > index 97e1641..90f406d 100644 > --- a/block/qapi.c > +++ b/block/qapi.c > @@ -165,19 +165,25 @@ void bdrv_query_image_info(BlockDriverState *bs, > ImageInfo **p_info, > Error **errp) > { > - uint64_t total_sectors; > + int64_t size; > const char *backing_filename; > char backing_filename2[1024]; > BlockDriverInfo bdi; > int ret; > Error *err =3D NULL; > - ImageInfo *info =3D g_new0(ImageInfo, 1); > + ImageInfo *info; > =20 > - bdrv_get_geometry(bs, &total_sectors); > + size =3D bdrv_getlength(bs); > + if (size < 0) { > + error_setg_errno(errp, -size, "Can't get size of device '%s'", > + bdrv_get_device_name(bs)); > + return; > + } > =20 > + info =3D g_new0(ImageInfo, 1); > info->filename =3D g_strdup(bs->filename); > info->format =3D g_strdup(bdrv_get_format_name(bs)); > - info->virtual_size =3D total_sectors * 512; > + info->virtual_size =3D size; > info->actual_size =3D bdrv_get_allocated_file_size(bs); > info->has_actual_size =3D info->actual_size >=3D 0; > if (bdrv_is_encrypted(bs)) { > diff --git a/qemu-img.c b/qemu-img.c > index e6d0edf..7e6dde0 100644 > --- a/qemu-img.c > +++ b/qemu-img.c > @@ -958,7 +958,6 @@ static int img_compare(int argc, char **argv) > int64_t sector_num =3D 0; > int64_t nb_sectors; > int c, pnum; > - uint64_t bs_sectors; > uint64_t progress_base; > =20 > for (;;) { > @@ -1020,10 +1019,20 @@ static int img_compare(int argc, char **argv) > =20 > buf1 =3D qemu_blockalign(bs1, IO_BUF_SIZE); > buf2 =3D qemu_blockalign(bs2, IO_BUF_SIZE); > - bdrv_get_geometry(bs1, &bs_sectors); > - total_sectors1 =3D bs_sectors; > - bdrv_get_geometry(bs2, &bs_sectors); > - total_sectors2 =3D bs_sectors; > + total_sectors1 =3D bdrv_nb_sectors(bs1); > + if (total_sectors1 < 0) { > + error_report("Can't get size of %s: %s", > + filename1, strerror(-total_sectors1)); > + ret =3D 4; > + goto out; > + } > + total_sectors2 =3D bdrv_nb_sectors(bs2); > + if (total_sectors2 < 0) { > + error_report("Can't get size of %s: %s", > + filename2, strerror(-total_sectors2)); > + ret =3D 4; > + goto out; > + } > total_sectors =3D MIN(total_sectors1, total_sectors2); > progress_base =3D MAX(total_sectors1, total_sectors2); > =20 > @@ -1185,7 +1194,7 @@ static int img_convert(int argc, char **argv) > BlockDriver *drv, *proto_drv; > BlockDriverState **bs =3D NULL, *out_bs =3D NULL; > int64_t total_sectors, nb_sectors, sector_num, bs_offset; > - uint64_t *bs_sectors =3D NULL; > + int64_t *bs_sectors =3D NULL; > uint8_t * buf =3D NULL; > size_t bufsectors =3D IO_BUF_SIZE / BDRV_SECTOR_SIZE; > const uint8_t *buf1; > @@ -1326,7 +1335,7 @@ static int img_convert(int argc, char **argv) > qemu_progress_print(0, 100); > =20 > bs =3D g_new0(BlockDriverState *, bs_n); > - bs_sectors =3D g_new(uint64_t, bs_n); > + bs_sectors =3D g_new(int64_t, bs_n); > =20 > total_sectors =3D 0; > for (bs_i =3D 0; bs_i < bs_n; bs_i++) { > @@ -1340,7 +1349,13 @@ static int img_convert(int argc, char **argv) > ret =3D -1; > goto out; > } > - bdrv_get_geometry(bs[bs_i], &bs_sectors[bs_i]); > + bs_sectors[bs_i] =3D bdrv_nb_sectors(bs[bs_i]); > + if (bs_sectors[bs_i] < 0) { > + error_report("Could not get size of %s: %s", > + argv[optind + bs_i], strerror(-bs_sectors[bs_= i])); > + ret =3D -1; > + goto out; > + } > total_sectors +=3D bs_sectors[bs_i]; > } > =20 > @@ -2421,9 +2436,9 @@ static int img_rebase(int argc, char **argv) > * the image is the same as the original one at any time. > */ > if (!unsafe) { > - uint64_t num_sectors; > - uint64_t old_backing_num_sectors; > - uint64_t new_backing_num_sectors =3D 0; > + int64_t num_sectors; > + int64_t old_backing_num_sectors; > + int64_t new_backing_num_sectors =3D 0; > uint64_t sector; > int n; > uint8_t * buf_old; > @@ -2433,10 +2448,31 @@ static int img_rebase(int argc, char **argv) > buf_old =3D qemu_blockalign(bs, IO_BUF_SIZE); > buf_new =3D qemu_blockalign(bs, IO_BUF_SIZE); > =20 > - bdrv_get_geometry(bs, &num_sectors); > - bdrv_get_geometry(bs_old_backing, &old_backing_num_sectors); > + num_sectors =3D bdrv_nb_sectors(bs); > + if (num_sectors < 0) { > + error_report("Could not get size of '%s': %s", > + filename, strerror(-num_sectors)); > + ret =3D -1; > + goto out; > + } > + old_backing_num_sectors =3D bdrv_nb_sectors(bs_old_backing); > + if (old_backing_num_sectors < 0) { > + char backing_name[1024]; Could you put this on the heap ? I recently fixed a stack overflow when taking snapshots due to multiple P= ATH_MAX char array in a recursive function. We don't know how this function will be used later. Best regards Beno=EEt > + > + bdrv_get_backing_filename(bs, backing_name, sizeof(backing= _name)); > + error_report("Could not get size of '%s': %s", > + backing_name, strerror(-old_backing_num_secto= rs)); > + ret =3D -1; > + goto out; > + } > if (bs_new_backing) { > - bdrv_get_geometry(bs_new_backing, &new_backing_num_sectors= ); > + new_backing_num_sectors =3D bdrv_nb_sectors(bs_new_backing= ); > + if (new_backing_num_sectors < 0) { > + error_report("Could not get size of '%s': %s", > + out_baseimg, strerror(-new_backing_num_se= ctors)); > + ret =3D -1; > + goto out; > + } > } > =20 > if (num_sectors !=3D 0) { > --=20 > 1.9.3 >=20