From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40045) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XRhdy-0000OT-7Y for qemu-devel@nongnu.org; Wed, 10 Sep 2014 09:09:12 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XRhdr-0003hX-6E for qemu-devel@nongnu.org; Wed, 10 Sep 2014 09:09:06 -0400 Received: from lputeaux-656-01-25-125.w80-12.abo.wanadoo.fr ([80.12.84.125]:41908 helo=paradis.irqsave.net) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XRhdq-0003gT-T1 for qemu-devel@nongnu.org; Wed, 10 Sep 2014 09:08:59 -0400 Date: Wed, 10 Sep 2014 15:08:03 +0200 From: =?iso-8859-1?Q?Beno=EEt?= Canet Message-ID: <20140910130803.GE22376@irqsave.net> References: <1410336832-22160-1-git-send-email-armbru@redhat.com> <1410336832-22160-5-git-send-email-armbru@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <1410336832-22160-5-git-send-email-armbru@redhat.com> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 04/23] block: Connect BlockBackend and DriveInfo List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: kwolf@redhat.com, benoit.canet@irqsave.net, famz@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com The Wednesday 10 Sep 2014 =E0 10:13:33 (+0200), Markus Armbruster wrote : > Make the BlockBackend own the DriveInfo. Change blockdev_init() to > return the BlockBackend instead of the DriveInfo. >=20 > Signed-off-by: Markus Armbruster > --- > block/block-backend.c | 38 +++++++++++++++++++++++ > blockdev.c | 79 +++++++++++++++++++++------------------= -------- > include/sysemu/blockdev.h | 4 +++ > 3 files changed, 78 insertions(+), 43 deletions(-) >=20 > diff --git a/block/block-backend.c b/block/block-backend.c > index deccb54..2a22660 100644 > --- a/block/block-backend.c > +++ b/block/block-backend.c > @@ -12,14 +12,18 @@ > =20 > #include "sysemu/block-backend.h" > #include "block/block_int.h" > +#include "sysemu/blockdev.h" > =20 > struct BlockBackend { > char *name; > int refcnt; > BlockDriverState *bs; > + DriveInfo *legacy_dinfo; > QTAILQ_ENTRY(BlockBackend) link; /* for blk_backends */ > }; > =20 > +static void drive_info_del(DriveInfo *dinfo); Is there any technical reason not to just put the drive_info_del above the blk_delete function ? I don't see any possible circular references between the two. Some people like Eric frown upon static function prototypes in final code that's why I am asking. > + > static QTAILQ_HEAD(, BlockBackend) blk_backends =3D > QTAILQ_HEAD_INITIALIZER(blk_backends); > =20 > @@ -87,6 +91,7 @@ static void blk_delete(BlockBackend *blk) > blk_detach_bs(blk); > QTAILQ_REMOVE(&blk_backends, blk, link); > g_free(blk->name); > + drive_info_del(blk->legacy_dinfo); > g_free(blk); > } > =20 > @@ -119,6 +124,16 @@ void blk_unref(BlockBackend *blk) > } > } > =20 > +static void drive_info_del(DriveInfo *dinfo) > +{ > + if (dinfo) { > + qemu_opts_del(dinfo->opts); > + g_free(dinfo->id); > + g_free(dinfo->serial); > + g_free(dinfo); > + } > +} > + > const char *blk_name(BlockBackend *blk) > { > return blk->name; > @@ -187,3 +202,26 @@ BlockDriverState *blk_detach_bs(BlockBackend *blk) > } > return bs; > } > + > +DriveInfo *blk_legacy_dinfo(BlockBackend *blk) > +{ > + return blk->legacy_dinfo; > +} > + > +DriveInfo *blk_set_legacy_dinfo(BlockBackend *blk, DriveInfo *dinfo) > +{ > + assert(!blk->legacy_dinfo); > + return blk->legacy_dinfo =3D dinfo; > +} > + > +BlockBackend *blk_by_legacy_dinfo(DriveInfo *dinfo) > +{ > + BlockBackend *blk; > + > + QTAILQ_FOREACH(blk, &blk_backends, link) { > + if (blk->legacy_dinfo =3D=3D dinfo) { > + return blk; > + } > + } > + assert(0); > +} > diff --git a/blockdev.c b/blockdev.c > index 0a0b95e..73e2da9 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -47,8 +47,6 @@ > #include "trace.h" > #include "sysemu/arch_init.h" > =20 > -static QTAILQ_HEAD(drivelist, DriveInfo) drives =3D QTAILQ_HEAD_INITIA= LIZER(drives); > - > static const char *const if_name[IF_COUNT] =3D { > [IF_NONE] =3D "none", > [IF_IDE] =3D "ide", > @@ -89,7 +87,8 @@ static const int if_max_devs[IF_COUNT] =3D { > */ > void blockdev_mark_auto_del(BlockDriverState *bs) > { > - DriveInfo *dinfo =3D drive_get_by_blockdev(bs); > + BlockBackend *blk =3D bs->blk; > + DriveInfo *dinfo =3D blk_legacy_dinfo(blk); > =20 > if (dinfo && !dinfo->enable_auto_del) { > return; > @@ -105,7 +104,8 @@ void blockdev_mark_auto_del(BlockDriverState *bs) > =20 > void blockdev_auto_del(BlockDriverState *bs) > { > - DriveInfo *dinfo =3D drive_get_by_blockdev(bs); > + BlockBackend *blk =3D bs->blk; > + DriveInfo *dinfo =3D blk_legacy_dinfo(blk); > =20 > if (dinfo && dinfo->auto_del) { > drive_del(dinfo); > @@ -153,15 +153,15 @@ QemuOpts *drive_add(BlockInterfaceType type, int = index, const char *file, > =20 > DriveInfo *drive_get(BlockInterfaceType type, int bus, int unit) > { > + BlockBackend *blk; > DriveInfo *dinfo; > =20 > - /* seek interface, bus and unit */ > - > - QTAILQ_FOREACH(dinfo, &drives, next) { > - if (dinfo->type =3D=3D type && > - dinfo->bus =3D=3D bus && > - dinfo->unit =3D=3D unit) > + for (blk =3D blk_next(NULL); blk; blk =3D blk_next(blk)) { Here I understand why you didn't made blk_next pure round robin circling = in a loop. Maybe the comments of the blk_next function should say it's just an itera= tor. > + dinfo =3D blk_legacy_dinfo(blk); > + if (dinfo && dinfo->type =3D=3D type > + && dinfo->bus =3D=3D bus && dinfo->unit =3D=3D unit) { > return dinfo; > + } > } > =20 > return NULL; > @@ -177,13 +177,15 @@ DriveInfo *drive_get_by_index(BlockInterfaceType = type, int index) > int drive_get_max_bus(BlockInterfaceType type) > { > int max_bus; > + BlockBackend *blk; > DriveInfo *dinfo; > =20 > max_bus =3D -1; > - QTAILQ_FOREACH(dinfo, &drives, next) { > - if(dinfo->type =3D=3D type && > - dinfo->bus > max_bus) > + for (blk =3D blk_next(NULL); blk; blk =3D blk_next(blk)) { > + dinfo =3D blk_legacy_dinfo(blk); > + if (dinfo && dinfo->type =3D=3D type && dinfo->bus > max_bus) = { > max_bus =3D dinfo->bus; > + } > } > return max_bus; > } > @@ -200,11 +202,11 @@ DriveInfo *drive_get_next(BlockInterfaceType type= ) > =20 > DriveInfo *drive_get_by_blockdev(BlockDriverState *bs) > { > - DriveInfo *dinfo; > + BlockBackend *blk; > =20 > - QTAILQ_FOREACH(dinfo, &drives, next) { > - if (dinfo->bdrv =3D=3D bs) { > - return dinfo; > + for (blk =3D blk_next(NULL); blk; blk =3D blk_next(blk)) { > + if (blk_bs(blk) =3D=3D bs) { > + return blk_legacy_dinfo(blk); > } > } > return NULL; > @@ -217,16 +219,10 @@ static void bdrv_format_print(void *opaque, const= char *name) > =20 > void drive_del(DriveInfo *dinfo) > { > - if (dinfo->opts) { > - qemu_opts_del(dinfo->opts); > - } > + BlockBackend *blk =3D dinfo->bdrv->blk; > =20 > bdrv_unref(dinfo->bdrv); > - blk_unref(blk_by_name(dinfo->id)); > - g_free(dinfo->id); > - QTAILQ_REMOVE(&drives, dinfo, next); > - g_free(dinfo->serial); > - g_free(dinfo); > + blk_unref(blk); > } > =20 > typedef struct { > @@ -296,8 +292,8 @@ static bool check_throttle_config(ThrottleConfig *c= fg, Error **errp) > typedef enum { MEDIA_DISK, MEDIA_CDROM } DriveMediaType; > =20 > /* Takes the ownership of bs_opts */ > -static DriveInfo *blockdev_init(const char *file, QDict *bs_opts, > - Error **errp) > +static BlockBackend *blockdev_init(const char *file, QDict *bs_opts, > + Error **errp) > { > const char *buf; > int ro =3D 0; > @@ -480,7 +476,7 @@ static DriveInfo *blockdev_init(const char *file, Q= Dict *bs_opts, > dinfo =3D g_malloc0(sizeof(*dinfo)); > dinfo->id =3D g_strdup(qemu_opts_id(opts)); > dinfo->bdrv =3D bs; > - QTAILQ_INSERT_TAIL(&drives, dinfo, next); > + blk_set_legacy_dinfo(blk, dinfo); > =20 > if (!file || !*file) { > if (has_driver_specific_opts) { > @@ -488,7 +484,7 @@ static DriveInfo *blockdev_init(const char *file, Q= Dict *bs_opts, > } else { > QDECREF(bs_opts); > qemu_opts_del(opts); > - return dinfo; > + return blk; Here you fix the leak I found on a previous patch. > } > } > if (snapshot) { > @@ -525,13 +521,10 @@ static DriveInfo *blockdev_init(const char *file,= QDict *bs_opts, > QDECREF(bs_opts); > qemu_opts_del(opts); > =20 > - return dinfo; > + return blk; > =20 > err: > bdrv_unref(dinfo->bdrv); > - QTAILQ_REMOVE(&drives, dinfo, next); > - g_free(dinfo->id); > - g_free(dinfo); > blk_unref(blk); > early_err: > qemu_opts_del(opts); > @@ -635,6 +628,7 @@ QemuOptsList qemu_legacy_drive_opts =3D { > DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_defa= ult_type) > { > const char *value; > + BlockBackend *blk; > DriveInfo *dinfo =3D NULL; > QDict *bs_opts; > QemuOpts *legacy_opts; > @@ -917,19 +911,17 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInt= erfaceType block_default_type) > } > =20 > /* Actual block device init: Functionality shared with blockdev-ad= d */ > - dinfo =3D blockdev_init(filename, bs_opts, &local_err); > + blk =3D blockdev_init(filename, bs_opts, &local_err); > bs_opts =3D NULL; > - if (dinfo =3D=3D NULL) { > - if (local_err) { > - error_report("%s", error_get_pretty(local_err)); > - error_free(local_err); > - } > + if (!blk) { Just down the code an existing test does if (local_err) { after blk =3D blockdev_init(NULL, qdict, &local_err);. Is the prefered convention checking blk or local_err ? > + error_report("%s", error_get_pretty(local_err)); > goto fail; > } else { > assert(!local_err); > } > =20 > /* Set legacy DriveInfo fields */ > + dinfo =3D blk_legacy_dinfo(blk); > dinfo->enable_auto_del =3D true; > dinfo->opts =3D all_opts; > =20 > @@ -2478,7 +2470,7 @@ void qmp_change_backing_file(const char *device, > void qmp_blockdev_add(BlockdevOptions *options, Error **errp) > { > QmpOutputVisitor *ov =3D qmp_output_visitor_new(); > - DriveInfo *dinfo; > + BlockBackend *blk; > QObject *obj; > QDict *qdict; > Error *local_err =3D NULL; > @@ -2516,14 +2508,15 @@ void qmp_blockdev_add(BlockdevOptions *options,= Error **errp) > =20 > qdict_flatten(qdict); > =20 > - dinfo =3D blockdev_init(NULL, qdict, &local_err); > + blk =3D blockdev_init(NULL, qdict, &local_err); > if (local_err) { > error_propagate(errp, local_err); > goto fail; > } > =20 > - if (bdrv_key_required(dinfo->bdrv)) { > - drive_del(dinfo); > + if (bdrv_key_required(blk_bs(blk))) { > + bdrv_unref(blk_bs(blk)); > + blk_unref(blk); > error_setg(errp, "blockdev-add doesn't support encrypted devic= es"); > goto fail; > } > diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h > index 23a5d10..2ed297b 100644 > --- a/include/sysemu/blockdev.h > +++ b/include/sysemu/blockdev.h > @@ -45,6 +45,10 @@ struct DriveInfo { > QTAILQ_ENTRY(DriveInfo) next; > }; > =20 > +DriveInfo *blk_legacy_dinfo(BlockBackend *blk); > +DriveInfo *blk_set_legacy_dinfo(BlockBackend *blk, DriveInfo *dinfo); > +BlockBackend *blk_by_legacy_dinfo(DriveInfo *dinfo); > + > DriveInfo *drive_get(BlockInterfaceType type, int bus, int unit); > DriveInfo *drive_get_by_index(BlockInterfaceType type, int index); > int drive_get_max_bus(BlockInterfaceType type); > --=20 > 1.9.3 >=20