From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33310) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XTUcr-0006bv-5U for qemu-devel@nongnu.org; Mon, 15 Sep 2014 07:39:29 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XTUcl-0007xC-Lw for qemu-devel@nongnu.org; Mon, 15 Sep 2014 07:39:21 -0400 Received: from lputeaux-656-01-25-125.w80-12.abo.wanadoo.fr ([80.12.84.125]:49869 helo=paradis.irqsave.net) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XTUcl-0007u4-4Y for qemu-devel@nongnu.org; Mon, 15 Sep 2014 07:39:15 -0400 Date: Mon, 15 Sep 2014 13:38:15 +0200 From: =?iso-8859-1?Q?Beno=EEt?= Canet Message-ID: <20140915113814.GB25752@irqsave.net> References: <1410549984-16110-1-git-send-email-armbru@redhat.com> <1410549984-16110-3-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-3-git-send-email-armbru@redhat.com> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 2/4] block: Keep DriveInfo alive until BlockDriverState dies 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:22 (+0200), Markus Armbruster wrote : > If the BDS's refcnt > 0, drive_del() destroys the DriveInfo, but not > the BDS. This can happen in three places: >=20 > * Device model destruction during unplug: blockdev_auto_del() >=20 > * Xen IDE unplug: pci_piix3_xen_ide_unplug() >=20 > * drive_del command when no device model is attached: do_drive_del() >=20 > The other callers of drive_del are on error paths where refcnt =3D=3D 1= . >=20 > If the user somehow manages to plug in a device model using a BDS that > has gone through drive_del(), the legacy configuration passed in > DriveInfo doesn't reach the device model, and automatic deletion on > unplug doesn't work. Worse, some device models such as scsi-disk > crash when DriveInfo doesn't exist. >=20 > This is theoretical; I didn't research an actual reproducer. >=20 > Fix by keeping DriveInfo alive until its BDS dies. >=20 > This affects qemu_drive_opts: now you can't reuse the same ID for new > drive options until the BDS dies. Before, you could, but since the > code always attempts to create a BDS with the same ID next, the > enclosing operation "create a new drive" failed anyway. Different > error path, same result. >=20 > Unfortunately, the fix involves use of blockdev.c stuff from block.c, > which is a layering violation. Fortunately, my forthcoming > BlockBackend work will get rid of it again. >=20 > Signed-off-by: Markus Armbruster > --- > block.c | 2 ++ > blockdev.c | 13 ++++++++----- > include/sysemu/blockdev.h | 1 + > stubs/Makefile.objs | 1 + > stubs/blockdev.c | 12 ++++++++++++ > 5 files changed, 24 insertions(+), 5 deletions(-) > create mode 100644 stubs/blockdev.c >=20 > diff --git a/block.c b/block.c > index d06dd51..6faf36f 100644 > --- a/block.c > +++ b/block.c > @@ -29,6 +29,7 @@ > #include "qemu/module.h" > #include "qapi/qmp/qjson.h" > #include "sysemu/sysemu.h" > +#include "sysemu/blockdev.h" /* FIXME layering violation */ > #include "qemu/notify.h" > #include "block/coroutine.h" > #include "block/qapi.h" > @@ -2110,6 +2111,7 @@ static void bdrv_delete(BlockDriverState *bs) > /* remove from list, if necessary */ > bdrv_make_anon(bs); > =20 > + drive_info_del(drive_get_by_blockdev(bs)); > g_free(bs); > } > =20 > diff --git a/blockdev.c b/blockdev.c > index 5ec4635..450f95c 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -216,11 +216,17 @@ static void bdrv_format_print(void *opaque, const= char *name) > =20 > void drive_del(DriveInfo *dinfo) > { > + bdrv_unref(dinfo->bdrv); > +} > + > +void drive_info_del(DriveInfo *dinfo) > +{ > + if (!dinfo) { > + return; > + } > if (dinfo->opts) { > qemu_opts_del(dinfo->opts); > } > - > - bdrv_unref(dinfo->bdrv); > g_free(dinfo->id); > QTAILQ_REMOVE(&drives, dinfo, next); > g_free(dinfo->serial); > @@ -525,9 +531,6 @@ static DriveInfo *blockdev_init(const char *file, Q= Dict *bs_opts, > =20 > err: > bdrv_unref(bs); > - QTAILQ_REMOVE(&drives, dinfo, next); > - g_free(dinfo->id); > - g_free(dinfo); > early_err: > qemu_opts_del(opts); > err_no_opts: > diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h > index 23a5d10..abec381 100644 > --- a/include/sysemu/blockdev.h > +++ b/include/sysemu/blockdev.h > @@ -56,6 +56,7 @@ QemuOpts *drive_add(BlockInterfaceType type, int inde= x, const char *file, > const char *optstr); > DriveInfo *drive_new(QemuOpts *arg, BlockInterfaceType block_default_t= ype); > void drive_del(DriveInfo *dinfo); > +void drive_info_del(DriveInfo *dinfo); > =20 > /* device-hotplug */ > =20 > diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs > index 5e347d0..c0b1f6a 100644 > --- a/stubs/Makefile.objs > +++ b/stubs/Makefile.objs > @@ -1,5 +1,6 @@ > stub-obj-y +=3D arch-query-cpu-def.o > stub-obj-y +=3D bdrv-commit-all.o > +stub-obj-y +=3D blockdev.o > stub-obj-y +=3D chr-baum-init.o > stub-obj-y +=3D chr-msmouse.o > stub-obj-y +=3D chr-testdev.o > diff --git a/stubs/blockdev.c b/stubs/blockdev.c > new file mode 100644 > index 0000000..5d0a79c > --- /dev/null > +++ b/stubs/blockdev.c > @@ -0,0 +1,12 @@ > +#include > +#include "sysemu/blockdev.h" > + > +DriveInfo *drive_get_by_blockdev(BlockDriverState *bs) > +{ > + return NULL; > +} > + > +void drive_info_del(DriveInfo *dinfo) > +{ > + assert(!dinfo); > +} > --=20 > 1.9.3 > Reviewed-by: Beno=EEt Canet