From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57857) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XTUIr-0007v8-Qk for qemu-devel@nongnu.org; Mon, 15 Sep 2014 07:18:46 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XTUIm-0001Vt-O4 for qemu-devel@nongnu.org; Mon, 15 Sep 2014 07:18:41 -0400 Received: from lputeaux-656-01-25-125.w80-12.abo.wanadoo.fr ([80.12.84.125]:49857 helo=paradis.irqsave.net) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XTUIm-0001Vi-DC for qemu-devel@nongnu.org; Mon, 15 Sep 2014 07:18:36 -0400 Date: Mon, 15 Sep 2014 13:17:35 +0200 From: =?iso-8859-1?Q?Beno=EEt?= Canet Message-ID: <20140915111734.GA25752@irqsave.net> References: <1410549984-16110-1-git-send-email-armbru@redhat.com> <1410549984-16110-2-git-send-email-armbru@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <1410549984-16110-2-git-send-email-armbru@redhat.com> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 1/4] blockdev: Disentangle BlockDriverState and DriveInfo creation List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: kwolf@redhat.com, benoit.canet@irqsave.net, qemu-devel@nongnu.org, stefanha@redhat.com The Friday 12 Sep 2014 =E0 21:26:21 (+0200), Markus Armbruster wrote : > blockdev_init() mixes up BlockDriverState and DriveInfo initialization > Finish the BlockDriverState job before starting to mess with > DriveInfo. Easier on the eyes. >=20 > Signed-off-by: Markus Armbruster > --- > blockdev.c | 43 +++++++++++++++++++++++-------------------- > 1 file changed, 23 insertions(+), 20 deletions(-) >=20 > diff --git a/blockdev.c b/blockdev.c > index b361fbb..5ec4635 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -301,6 +301,7 @@ static DriveInfo *blockdev_init(const char *file, Q= Dict *bs_opts, > int ro =3D 0; > int bdrv_flags =3D 0; > int on_read_error, on_write_error; > + BlockDriverState *bs; > DriveInfo *dinfo; > ThrottleConfig cfg; > int snapshot =3D 0; > @@ -456,26 +457,27 @@ static DriveInfo *blockdev_init(const char *file,= QDict *bs_opts, > } > =20 > /* init */ > + bs =3D bdrv_new(qemu_opts_id(opts), errp); > + if (!bs) { > + goto early_err; > + } > + bs->open_flags =3D snapshot ? BDRV_O_SNAPSHOT : 0; > + bs->read_only =3D ro; > + bs->detect_zeroes =3D detect_zeroes; > + > + bdrv_set_on_error(bs, on_read_error, on_write_error); > + > + /* disk I/O throttling */ > + if (throttle_enabled(&cfg)) { > + bdrv_io_limits_enable(bs); > + bdrv_set_io_limits(bs, &cfg); > + } > + > dinfo =3D g_malloc0(sizeof(*dinfo)); > dinfo->id =3D g_strdup(qemu_opts_id(opts)); > - dinfo->bdrv =3D bdrv_new(dinfo->id, &error); > - if (error) { > - error_propagate(errp, error); > - goto bdrv_new_err; > - } > - dinfo->bdrv->open_flags =3D snapshot ? BDRV_O_SNAPSHOT : 0; > - dinfo->bdrv->read_only =3D ro; > - dinfo->bdrv->detect_zeroes =3D detect_zeroes; > + dinfo->bdrv =3D bs; > QTAILQ_INSERT_TAIL(&drives, dinfo, next); > =20 > - bdrv_set_on_error(dinfo->bdrv, on_read_error, on_write_error); > - > - /* disk I/O throttling */ > - if (throttle_enabled(&cfg)) { > - bdrv_io_limits_enable(dinfo->bdrv); > - bdrv_set_io_limits(dinfo->bdrv, &cfg); > - } > - > if (!file || !*file) { > if (has_driver_specific_opts) { > file =3D NULL; > @@ -502,7 +504,8 @@ static DriveInfo *blockdev_init(const char *file, Q= Dict *bs_opts, > bdrv_flags |=3D ro ? 0 : BDRV_O_RDWR; > =20 > QINCREF(bs_opts); > - ret =3D bdrv_open(&dinfo->bdrv, file, NULL, bs_opts, bdrv_flags, d= rv, &error); > + ret =3D bdrv_open(&bs, file, NULL, bs_opts, bdrv_flags, drv, &erro= r); > + assert(bs =3D=3D dinfo->bdrv); > =20 > if (ret < 0) { > error_setg(errp, "could not open disk image %s: %s", > @@ -511,8 +514,9 @@ static DriveInfo *blockdev_init(const char *file, Q= Dict *bs_opts, > goto err; > } > =20 > - if (bdrv_key_required(dinfo->bdrv)) > + if (bdrv_key_required(bs)) { > autostart =3D 0; > + } > =20 > QDECREF(bs_opts); > qemu_opts_del(opts); > @@ -520,9 +524,8 @@ static DriveInfo *blockdev_init(const char *file, Q= Dict *bs_opts, > return dinfo; > =20 > err: > - bdrv_unref(dinfo->bdrv); > + bdrv_unref(bs); I would have moved this. > QTAILQ_REMOVE(&drives, dinfo, next); > -bdrv_new_err: > g_free(dinfo->id); > g_free(dinfo); To Here. No functional difference but it would reflect it's goto chain role: being in the reverse order of the allocations. > early_err: > --=20 > 1.9.3 Reviewed-by: Beno=EEt Canet >=20