[PATCH] xen-block: Avoid leaks on new error path
From: Anthony PERARD
Commit 189829399070 ("xen-block: Use specific blockdev driver")
introduced a new error path, without taking care of allocated
resources.
So only allocate the qdicts after the error check, and free both
`filename` and `driver` when we are about to return and thus taking
care of both success and error path.
Coverity only spotted the leak of qdicts (*_layer variables).
Reported-by: Peter Maydell
Fixes: Coverity CID 1508722, 1398649
Fixes: 189829399070 ("xen-block: Use specific blockdev driver")
Signed-off-by: Anthony PERARD
---
hw/block/xen-block.c | 11 ++-
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c
index f099914831..3906b9058b 100644
--- a/hw/block/xen-block.c
+++ b/hw/block/xen-block.c
@@ -781,14 +781,15 @@ static XenBlockDrive *xen_block_drive_create(const char
*id,
drive = g_new0(XenBlockDrive, 1);
drive->id = g_strdup(id);
-file_layer = qdict_new();
-driver_layer = qdict_new();
-
rc = stat(filename, &st);
if (rc) {
error_setg_errno(errp, errno, "Could not stat file '%s'", filename);
goto done;
}
+
+file_layer = qdict_new();
+driver_layer = qdict_new();
+
if (S_ISBLK(st.st_mode)) {
qdict_put_str(file_layer, "driver", "host_device");
} else {
@@ -796,7 +797,6 @@ static XenBlockDrive *xen_block_drive_create(const char *id,
}
qdict_put_str(file_layer, "filename", filename);
-g_free(filename);
if (mode && *mode != 'w') {
qdict_put_bool(file_layer, "read-only", true);
@@ -831,7 +831,6 @@ static XenBlockDrive *xen_block_drive_create(const char *id,
qdict_put_str(file_layer, "locking", "off");
qdict_put_str(driver_layer, "driver", driver);
-g_free(driver);
qdict_put(driver_layer, "file", file_layer);
@@ -842,6 +841,8 @@ static XenBlockDrive *xen_block_drive_create(const char *id,
qobject_unref(driver_layer);
done:
+g_free(filename);
+g_free(driver);
if (*errp) {
xen_block_drive_destroy(drive, NULL);
return NULL;
--
Anthony PERARD
Re: [PATCH v5 12/21] xen-block: implement BlockDevOps->drained_begin()
On Thu, May 04, 2023 at 03:53:18PM -0400, Stefan Hajnoczi wrote:
> @@ -819,11 +841,9 @@ void xen_block_dataplane_start(XenBlockDataPlane
> *dataplane,
> blk_set_aio_context(dataplane->blk, dataplane->ctx, NULL);
> aio_context_release(old_context);
>
> -/* Only reason for failure is a NULL channel */
> -aio_context_acquire(dataplane->ctx);
> -xen_device_set_event_channel_context(xendev, dataplane->event_channel,
> - dataplane->ctx, &error_abort);
> -aio_context_release(dataplane->ctx);
> +if (!blk_in_drain(dataplane->blk)) {
There's maybe something missing in the patch.
xen_block_dataplane_start() calls xen_device_bind_event_channel() just
before xen_block_dataplane_attach().
And xen_device_bind_event_channel() sets the event context to
qemu_get_aio_context() instead of NULL.
So, even if we don't call xen_block_dataplane_attach() while in drain,
there's already a fd_handler attach to the fd. So should
xen_device_bind_event_channel() be changed as well? Or maybe a call to
xen_block_dataplane_detach() would be enough.
(There's only one user of xen_device_bind_event_channel() at the moment
so I don't know if other implementation making use of this API will want
to call set_event_channel_context or not.)
> +xen_block_dataplane_attach(dataplane);
> +}
>
> return;
>
--
Anthony PERARD
Re: [PATCH v5 13/21] hw/xen: do not set is_external=true on evtchn fds
On Thu, May 04, 2023 at 03:53:19PM -0400, Stefan Hajnoczi wrote: > is_external=true suspends fd handlers between aio_disable_external() and > aio_enable_external(). The block layer's drain operation uses this > mechanism to prevent new I/O from sneaking in between > bdrv_drained_begin() and bdrv_drained_end(). > > The previous commit converted the xen-block device to use BlockDevOps > .drained_begin/end() callbacks. It no longer relies on is_external=true > so it is safe to pass is_external=false. > > This is part of ongoing work to remove the aio_disable_external() API. > > Signed-off-by: Stefan Hajnoczi Acked-by: Anthony PERARD Thanks, -- Anthony PERARD
Re: [PATCH v5 12/21] xen-block: implement BlockDevOps->drained_begin()
On Thu, May 04, 2023 at 03:53:18PM -0400, Stefan Hajnoczi wrote: > Detach event channels during drained sections to stop I/O submission > from the ring. xen-block is no longer reliant on aio_disable_external() > after this patch. This will allow us to remove the > aio_disable_external() API once all other code that relies on it is > converted. > > Extend xen_device_set_event_channel_context() to allow ctx=NULL. The > event channel still exists but the event loop does not monitor the file > descriptor. Event channel processing can resume by calling > xen_device_set_event_channel_context() with a non-NULL ctx. > > Factor out xen_device_set_event_channel_context() calls in > hw/block/dataplane/xen-block.c into attach/detach helper functions. > Incidentally, these don't require the AioContext lock because > aio_set_fd_handler() is thread-safe. > > It's safer to register BlockDevOps after the dataplane instance has been > created. The BlockDevOps .drained_begin/end() callbacks depend on the > dataplane instance, so move the blk_set_dev_ops() call after > xen_block_dataplane_create(). > > Signed-off-by: Stefan Hajnoczi Reviewed-by: Anthony PERARD Thanks, -- Anthony PERARD
Re: [PATCH 2/6] hw/xen: use G_GNUC_PRINTF/SCANF for various functions
On Mon, Dec 19, 2022 at 08:02:01AM -0500, Daniel P. Berrangé wrote: > Signed-off-by: Daniel P. Berrangé Acked-by: Anthony PERARD Thanks, -- Anthony PERARD
[PULL 3/3] include/hw/ide: Unexport pci_piix3_xen_ide_unplug()
From: Bernhard Beschow
This function was declared in a generic and public header, implemented
in a device-specific source file but only used in xen_platform. Given its
'aux' parameter, this function is more xen-specific than piix-specific.
Also, the hardcoded magic constants seem to be generic and related to
PCIIDEState and IDEBus rather than piix.
Therefore, move this function to xen_platform, unexport it, and drop the
"piix3" in the function name as well.
Signed-off-by: Bernhard Beschow
Reviewed-by: Paul Durrant
Acked-by: Anthony PERARD
Reviewed-by: Philippe Mathieu-Daudé
Message-Id: <20220513180957.90514-4-shen...@gmail.com>
Signed-off-by: Anthony PERARD
---
hw/i386/xen/xen_platform.c | 48 +-
hw/ide/piix.c | 46
include/hw/ide.h | 3 ---
3 files changed, 47 insertions(+), 50 deletions(-)
diff --git a/hw/i386/xen/xen_platform.c b/hw/i386/xen/xen_platform.c
index 72028449ba..a64265cca0 100644
--- a/hw/i386/xen/xen_platform.c
+++ b/hw/i386/xen/xen_platform.c
@@ -26,6 +26,7 @@
#include "qemu/osdep.h"
#include "qapi/error.h"
#include "hw/ide.h"
+#include "hw/ide/pci.h"
#include "hw/pci/pci.h"
#include "hw/xen/xen_common.h"
#include "migration/vmstate.h"
@@ -134,6 +135,51 @@ static void pci_unplug_nics(PCIBus *bus)
pci_for_each_device(bus, 0, unplug_nic, NULL);
}
+/*
+ * The Xen HVM unplug protocol [1] specifies a mechanism to allow guests to
+ * request unplug of 'aux' disks (which is stated to mean all IDE disks,
+ * except the primary master).
+ *
+ * NOTE: The semantics of what happens if unplug of all disks and 'aux' disks
+ * is simultaneously requested is not clear. The implementation assumes
+ * that an 'all' request overrides an 'aux' request.
+ *
+ * [1]
https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=docs/misc/hvm-emulated-unplug.pandoc
+ */
+static void pci_xen_ide_unplug(DeviceState *dev, bool aux)
+{
+PCIIDEState *pci_ide;
+int i;
+IDEDevice *idedev;
+IDEBus *idebus;
+BlockBackend *blk;
+
+pci_ide = PCI_IDE(dev);
+
+for (i = aux ? 1 : 0; i < 4; i++) {
+idebus = &pci_ide->bus[i / 2];
+blk = idebus->ifs[i % 2].blk;
+
+if (blk && idebus->ifs[i % 2].drive_kind != IDE_CD) {
+if (!(i % 2)) {
+idedev = idebus->master;
+} else {
+idedev = idebus->slave;
+}
+
+blk_drain(blk);
+blk_flush(blk);
+
+blk_detach_dev(blk, DEVICE(idedev));
+idebus->ifs[i % 2].blk = NULL;
+idedev->conf.blk = NULL;
+monitor_remove_blk(blk);
+blk_unref(blk);
+}
+}
+qdev_reset_all(dev);
+}
+
static void unplug_disks(PCIBus *b, PCIDevice *d, void *opaque)
{
uint32_t flags = *(uint32_t *)opaque;
@@ -147,7 +193,7 @@ static void unplug_disks(PCIBus *b, PCIDevice *d, void
*opaque)
switch (pci_get_word(d->config + PCI_CLASS_DEVICE)) {
case PCI_CLASS_STORAGE_IDE:
-pci_piix3_xen_ide_unplug(DEVICE(d), aux);
+pci_xen_ide_unplug(DEVICE(d), aux);
break;
case PCI_CLASS_STORAGE_SCSI:
diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index bc1b37512a..9a9b28078e 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -173,52 +173,6 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error
**errp)
}
}
-/*
- * The Xen HVM unplug protocol [1] specifies a mechanism to allow guests to
- * request unplug of 'aux' disks (which is stated to mean all IDE disks,
- * except the primary master).
- *
- * NOTE: The semantics of what happens if unplug of all disks and 'aux' disks
- * is simultaneously requested is not clear. The implementation assumes
- * that an 'all' request overrides an 'aux' request.
- *
- * [1]
https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=docs/misc/hvm-emulated-unplug.pandoc
- */
-int pci_piix3_xen_ide_unplug(DeviceState *dev, bool aux)
-{
-PCIIDEState *pci_ide;
-int i;
-IDEDevice *idedev;
-IDEBus *idebus;
-BlockBackend *blk;
-
-pci_ide = PCI_IDE(dev);
-
-for (i = aux ? 1 : 0; i < 4; i++) {
-idebus = &pci_ide->bus[i / 2];
-blk = idebus->ifs[i % 2].blk;
-
-if (blk && idebus->ifs[i % 2].drive_kind != IDE_CD) {
-if (!(i % 2)) {
-idedev = idebus->master;
-} else {
-idedev = idebus->slave;
-}
-
-blk_drain(blk);
-blk_flush(blk);
-
-blk_detach_dev(blk, DEVICE(idedev));
-idebus->ifs[i % 2].blk = NULL;
-idedev->conf.blk = NULL;
-monitor_remove_blk(blk);
-blk_unref(blk);
-}
-}
-qdev_reset_all(dev);
-return 0;
-}
-
static void pci_piix_ide_exitfn(PCIDevice *dev)
{
PCIIDEState *d = PCI_IDE(dev);
diff --git a/include/hw/ide.h b/include/hw/ide.h
index c5ce5da4f4..60f1f4f714 100644
--- a/include/hw/ide.h
+++ b/
[PULL 1/3] hw/ide/piix: Remove redundant "piix3-ide-xen" device class
From: Bernhard Beschow
Commit 0f8445820f11a69154309863960328dda3dc1ad4 'xen: piix reuse pci
generic class init function' already resolved redundant code which in
turn rendered piix3-ide-xen redundant.
Signed-off-by: Bernhard Beschow
Reviewed-by: Anthony PERARD
Message-Id: <20220513180957.90514-2-shen...@gmail.com>
Signed-off-by: Anthony PERARD
---
hw/i386/pc_piix.c | 3 +--
hw/ide/piix.c | 7 ---
2 files changed, 1 insertion(+), 9 deletions(-)
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 578e537b35..0e45521e74 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -246,8 +246,7 @@ static void pc_init1(MachineState *machine,
if (pcmc->pci_enabled) {
PCIDevice *dev;
-dev = pci_create_simple(pci_bus, piix3_devfn + 1,
-xen_enabled() ? "piix3-ide-xen" : "piix3-ide");
+dev = pci_create_simple(pci_bus, piix3_devfn + 1, "piix3-ide");
pci_ide_create_devs(dev);
idebus[0] = qdev_get_child_bus(&dev->qdev, "ide.0");
idebus[1] = qdev_get_child_bus(&dev->qdev, "ide.1");
diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index ce89fd0aa3..2345fe9e1d 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -241,12 +241,6 @@ static const TypeInfo piix3_ide_info = {
.class_init= piix3_ide_class_init,
};
-static const TypeInfo piix3_ide_xen_info = {
-.name = "piix3-ide-xen",
-.parent= TYPE_PCI_IDE,
-.class_init= piix3_ide_class_init,
-};
-
/* NOTE: for the PIIX4, the IRQs and IOports are hardcoded */
static void piix4_ide_class_init(ObjectClass *klass, void *data)
{
@@ -272,7 +266,6 @@ static const TypeInfo piix4_ide_info = {
static void piix_ide_register_types(void)
{
type_register_static(&piix3_ide_info);
-type_register_static(&piix3_ide_xen_info);
type_register_static(&piix4_ide_info);
}
--
Anthony PERARD
[PULL 2/3] hw/ide/piix: Add some documentation to pci_piix3_xen_ide_unplug()
From: Bernhard Beschow
The comment is based on commit message
ae4d2eb273b167dad748ea4249720319240b1ac2 'xen-platform: add missing disk
unplug option'. Since it seems to describe design decisions and
limitations that still apply it seems worth having.
Signed-off-by: Bernhard Beschow
Reviewed-by: Anthony PERARD
Message-Id: <20220513180957.90514-3-shen...@gmail.com>
Signed-off-by: Anthony PERARD
---
hw/ide/piix.c | 11 +++
1 file changed, 11 insertions(+)
diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index 2345fe9e1d..bc1b37512a 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -173,6 +173,17 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error
**errp)
}
}
+/*
+ * The Xen HVM unplug protocol [1] specifies a mechanism to allow guests to
+ * request unplug of 'aux' disks (which is stated to mean all IDE disks,
+ * except the primary master).
+ *
+ * NOTE: The semantics of what happens if unplug of all disks and 'aux' disks
+ * is simultaneously requested is not clear. The implementation assumes
+ * that an 'all' request overrides an 'aux' request.
+ *
+ * [1]
https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=docs/misc/hvm-emulated-unplug.pandoc
+ */
int pci_piix3_xen_ide_unplug(DeviceState *dev, bool aux)
{
PCIIDEState *pci_ide;
--
Anthony PERARD
Re: [PATCH v2 3/3] include/hw/ide: Unexport pci_piix3_xen_ide_unplug()
On Fri, May 13, 2022 at 08:09:57PM +0200, Bernhard Beschow wrote: > This function was declared in a generic and public header, implemented > in a device-specific source file but only used in xen_platform. Given its > 'aux' parameter, this function is more xen-specific than piix-specific. > Also, the hardcoded magic constants seem to be generic and related to > PCIIDEState and IDEBus rather than piix. > > Therefore, move this function to xen_platform, unexport it, and drop the > "piix3" in the function name as well. > > Signed-off-by: Bernhard Beschow > Reviewed-by: Paul Durrant Acked-by: Anthony PERARD Thanks, -- Anthony PERARD
Re: [PATCH v2 2/3] hw/ide/piix: Add some documentation to pci_piix3_xen_ide_unplug()
On Fri, May 13, 2022 at 08:09:56PM +0200, Bernhard Beschow wrote:
> The comment is based on commit message
> ae4d2eb273b167dad748ea4249720319240b1ac2 'xen-platform: add missing disk
> unplug option'. Since it seems to describe design decisions and
> limitations that still apply it seems worth having.
>
> Signed-off-by: Bernhard Beschow
> ---
> diff --git a/hw/ide/piix.c b/hw/ide/piix.c
> index 2345fe9e1d..bc1b37512a 100644
> --- a/hw/ide/piix.c
> +++ b/hw/ide/piix.c
> @@ -173,6 +173,17 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error
> **errp)
> }
> }
>
> +/*
> + * The Xen HVM unplug protocol [1] specifies a mechanism to allow guests to
> + * request unplug of 'aux' disks (which is stated to mean all IDE disks,
> + * except the primary master).
> + *
> + * NOTE: The semantics of what happens if unplug of all disks and 'aux' disks
> + * is simultaneously requested is not clear. The implementation assumes
> + * that an 'all' request overrides an 'aux' request.
> + *
> + * [1]
> https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=docs/misc/hvm-emulated-unplug.pandoc
> + */
> int pci_piix3_xen_ide_unplug(DeviceState *dev, bool aux)
> {
> PCIIDEState *pci_ide;
That comments seems to focus on 'aux', but it also gives some pointer on
what calls the function. So it looks fine.
Reviewed-by: Anthony PERARD
Thanks,
--
Anthony PERARD
Re: [PATCH v2 1/3] hw/ide/piix: Remove redundant "piix3-ide-xen" device class
On Fri, May 13, 2022 at 08:09:55PM +0200, Bernhard Beschow wrote: > Commit 0f8445820f11a69154309863960328dda3dc1ad4 'xen: piix reuse pci > generic class init function' already resolved redundant code which in > turn rendered piix3-ide-xen redundant. > > Signed-off-by: Bernhard Beschow Creating a guest and migrating a guest seems to work fine without "piix3-ide-xen", and I can't find this name used outside of QEMU. So I guess it's fine to remove it. Reviewed-by: Anthony PERARD Thanks, -- Anthony PERARD
[PATCH] xen-block: Use specific blockdev driver
From: Anthony PERARD
... when a xen-block backend instance is created via xenstore.
Following 8d17adf34f50 ("block: remove support for using "file" driver
with block/char devices"), using the "file" blockdev driver for
everything doesn't work anymore, we need to use the "host_device"
driver when the disk image is a block device and "file" driver when it
is a regular file.
Signed-off-by: Anthony PERARD
---
hw/block/xen-block.c | 14 +-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c
index 83754a434481..674953f1adee 100644
--- a/hw/block/xen-block.c
+++ b/hw/block/xen-block.c
@@ -728,6 +728,8 @@ static XenBlockDrive *xen_block_drive_create(const char *id,
XenBlockDrive *drive = NULL;
QDict *file_layer;
QDict *driver_layer;
+struct stat st;
+int rc;
if (params) {
char **v = g_strsplit(params, ":", 2);
@@ -761,7 +763,17 @@ static XenBlockDrive *xen_block_drive_create(const char
*id,
file_layer = qdict_new();
driver_layer = qdict_new();
-qdict_put_str(file_layer, "driver", "file");
+rc = stat(filename, &st);
+if (rc) {
+error_setg_errno(errp, errno, "Could not stat file '%s'", filename);
+goto done;
+}
+if (S_ISBLK(st.st_mode)) {
+qdict_put_str(file_layer, "driver", "host_device");
+} else {
+qdict_put_str(file_layer, "driver", "file");
+}
+
qdict_put_str(file_layer, "filename", filename);
g_free(filename);
--
Anthony PERARD
Re: [PATCH] xen-block: Fix removal of backend instance via xenstore
Hi Paul, Stefano,
Could one of you could give a Ack to this patch?
Thanks,
On Mon, Mar 08, 2021 at 02:32:32PM +, Anthony PERARD wrote:
> From: Anthony PERARD
>
> Whenever a Xen block device is detach via xenstore, the image
> associated with it remained open by the backend QEMU and an error is
> logged:
> qemu-system-i386: failed to destroy drive: Node xvdz-qcow2 is in use
>
> This happened since object_unparent() doesn't immediately frees the
> object and thus keep a reference to the node we are trying to free.
> The reference is hold by the "drive" property and the call
> xen_block_drive_destroy() fails.
>
> In order to fix that, we call drain_call_rcu() to run the callback
> setup by bus_remove_child() via object_unparent().
>
> Fixes: 2d24a6466154 ("device-core: use RCU for list of children of a bus")
>
> Signed-off-by: Anthony PERARD
> ---
> CCing people whom introduced/reviewed the change to use RCU to give
> them a chance to say if the change is fine.
> ---
> hw/block/xen-block.c | 9 +
> 1 file changed, 9 insertions(+)
>
> diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c
> index a3b69e27096f..fe5f828e2d25 100644
> --- a/hw/block/xen-block.c
> +++ b/hw/block/xen-block.c
> @@ -972,6 +972,15 @@ static void xen_block_device_destroy(XenBackendInstance
> *backend,
>
> object_unparent(OBJECT(xendev));
>
> +/*
> + * Drall all pending RCU callbacks as object_unparent() frees `xendev'
> + * in a RCU callback.
> + * And due to the property "drive" still existing in `xendev', we
> + * cann't destroy the XenBlockDrive associated with `xendev' with
> + * xen_block_drive_destroy() below.
> + */
> +drain_call_rcu();
> +
> if (iothread) {
> xen_block_iothread_destroy(iothread, errp);
> if (*errp) {
> --
> Anthony PERARD
>
--
Anthony PERARD
Re: [PATCH] xen-block: Fix removal of backend instance via xenstore
On Mon, Mar 08, 2021 at 06:37:38PM +0100, Paolo Bonzini wrote:
> On 08/03/21 18:29, Anthony PERARD wrote:
> > > If nothing else works then I guess it's okay, but why can't you do the
> > > xen_block_drive_destroy from e.g. an unrealize callback?
> >
> > I'm not sure if that's possible.
> >
> > xen_block_device_create/xen_block_device_destroy() is supposed to be
> > equivalent to do those qmp commands:
> > blockdev-add node-name=xvdz-qcow2 driver=qcow2
> > file={"driver":"file","filename":"disk.qcow2","locking":"off"}
> > device_add id=xvdz driver=xen-disk vdev=xvdz drive=xvdz-qcow2
> >
> > But I tried to add a call xen_block_drive_destroy from
> > xen_block_unrealize, but that still is called too early, it's called
> > before object_property_del_all() which would delete "drive" and call
> > release_drive() which would free the node.
>
> Can you use blockdev_mark_auto_del? Then you don't have to call
> xen_block_drive_destroy at all.
There is no legacy_dinfo, so blockdev_mark_auto_del doesn't work.
--
Anthony PERARD
Re: [PATCH] xen-block: Fix removal of backend instance via xenstore
On Mon, Mar 08, 2021 at 03:38:49PM +0100, Paolo Bonzini wrote:
> On 08/03/21 15:32, Anthony PERARD wrote:
> > From: Anthony PERARD
> >
> > Whenever a Xen block device is detach via xenstore, the image
> > associated with it remained open by the backend QEMU and an error is
> > logged:
> > qemu-system-i386: failed to destroy drive: Node xvdz-qcow2 is in use
> >
> > This happened since object_unparent() doesn't immediately frees the
> > object and thus keep a reference to the node we are trying to free.
> > The reference is hold by the "drive" property and the call
> > xen_block_drive_destroy() fails.
> >
> > In order to fix that, we call drain_call_rcu() to run the callback
> > setup by bus_remove_child() via object_unparent().
> >
> > Fixes: 2d24a6466154 ("device-core: use RCU for list of children of a bus")
> >
> > Signed-off-by: Anthony PERARD
> > ---
> > CCing people whom introduced/reviewed the change to use RCU to give
> > them a chance to say if the change is fine.
>
> If nothing else works then I guess it's okay, but why can't you do the
> xen_block_drive_destroy from e.g. an unrealize callback?
I'm not sure if that's possible.
xen_block_device_create/xen_block_device_destroy() is supposed to be
equivalent to do those qmp commands:
blockdev-add node-name=xvdz-qcow2 driver=qcow2
file={"driver":"file","filename":"disk.qcow2","locking":"off"}
device_add id=xvdz driver=xen-disk vdev=xvdz drive=xvdz-qcow2
But I tried to add a call xen_block_drive_destroy from
xen_block_unrealize, but that still is called too early, it's called
before object_property_del_all() which would delete "drive" and call
release_drive() which would free the node.
So, no, I don't think we can use an unrealized callback.
I though of trying to delete the "drive" property ahead of calling
object_unparent() but I didn't figure out how to do so and it's maybe
not possible.
So either drain_call_rcu or adding call_rcu(xen_block_drive_destroy)
seems to be the way, but since xen_block_drive_destroy uses
qmp_blockdev_del, it seems better to drain_call_rcu.
Cheers,
--
Anthony PERARD
[PATCH] xen-block: Fix removal of backend instance via xenstore
From: Anthony PERARD
Whenever a Xen block device is detach via xenstore, the image
associated with it remained open by the backend QEMU and an error is
logged:
qemu-system-i386: failed to destroy drive: Node xvdz-qcow2 is in use
This happened since object_unparent() doesn't immediately frees the
object and thus keep a reference to the node we are trying to free.
The reference is hold by the "drive" property and the call
xen_block_drive_destroy() fails.
In order to fix that, we call drain_call_rcu() to run the callback
setup by bus_remove_child() via object_unparent().
Fixes: 2d24a6466154 ("device-core: use RCU for list of children of a bus")
Signed-off-by: Anthony PERARD
---
CCing people whom introduced/reviewed the change to use RCU to give
them a chance to say if the change is fine.
---
hw/block/xen-block.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c
index a3b69e27096f..fe5f828e2d25 100644
--- a/hw/block/xen-block.c
+++ b/hw/block/xen-block.c
@@ -972,6 +972,15 @@ static void xen_block_device_destroy(XenBackendInstance
*backend,
object_unparent(OBJECT(xendev));
+/*
+ * Drall all pending RCU callbacks as object_unparent() frees `xendev'
+ * in a RCU callback.
+ * And due to the property "drive" still existing in `xendev', we
+ * cann't destroy the XenBlockDrive associated with `xendev' with
+ * xen_block_drive_destroy() below.
+ */
+drain_call_rcu();
+
if (iothread) {
xen_block_iothread_destroy(iothread, errp);
if (*errp) {
--
Anthony PERARD
Re: [PATCH v2] xen: rework pci_piix3_xen_ide_unplug
On Tue, Oct 27, 2020 at 01:33:32PM -0400, John Snow wrote: > On 10/27/20 11:40 AM, Anthony PERARD wrote: > > From: Anthony PERARD > > > > This is to allow IDE disks to be unplugged when adding to QEMU via: > > -drive file=/root/disk_file,if=none,id=ide-disk0,format=raw > > -device ide-hd,drive=ide-disk0,bus=ide.0,unit=0 > > > > as the current code only works for disk added with: > > -drive file=/root/disk_file,if=ide,index=0,media=disk,format=raw > > > > Since the code already have the IDE controller as `dev`, we don't need > > to use the legacy DriveInfo to find all the drive we want to unplug. > > We can simply use `blk` from the controller, as it kind of was already > > assume to be the same, by setting it to NULL. > > > > Signed-off-by: Anthony PERARD > > Acked-by: John Snow > > Do you need me to send a PR for this? No, that's fine, I can do the PR since it's all xen code. Thanks, -- Anthony PERARD
[PATCH v2] xen: rework pci_piix3_xen_ide_unplug
From: Anthony PERARD
This is to allow IDE disks to be unplugged when adding to QEMU via:
-drive file=/root/disk_file,if=none,id=ide-disk0,format=raw
-device ide-hd,drive=ide-disk0,bus=ide.0,unit=0
as the current code only works for disk added with:
-drive file=/root/disk_file,if=ide,index=0,media=disk,format=raw
Since the code already have the IDE controller as `dev`, we don't need
to use the legacy DriveInfo to find all the drive we want to unplug.
We can simply use `blk` from the controller, as it kind of was already
assume to be the same, by setting it to NULL.
Signed-off-by: Anthony PERARD
---
v2: coding style
CC: Paul Durrant
CC: Stefano Stabellini
---
hw/ide/piix.c | 27 +--
1 file changed, 13 insertions(+), 14 deletions(-)
diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index b402a936362b..b9860e35a5c4 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -164,30 +164,29 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error
**errp)
int pci_piix3_xen_ide_unplug(DeviceState *dev, bool aux)
{
PCIIDEState *pci_ide;
-DriveInfo *di;
int i;
IDEDevice *idedev;
+IDEBus *idebus;
+BlockBackend *blk;
pci_ide = PCI_IDE(dev);
for (i = aux ? 1 : 0; i < 4; i++) {
-di = drive_get_by_index(IF_IDE, i);
-if (di != NULL && !di->media_cd) {
-BlockBackend *blk = blk_by_legacy_dinfo(di);
-DeviceState *ds = blk_get_attached_dev(blk);
+idebus = &pci_ide->bus[i / 2];
+blk = idebus->ifs[i % 2].blk;
-blk_drain(blk);
-blk_flush(blk);
-
-if (ds) {
-blk_detach_dev(blk, ds);
-}
-pci_ide->bus[di->bus].ifs[di->unit].blk = NULL;
+if (blk && idebus->ifs[i % 2].drive_kind != IDE_CD) {
if (!(i % 2)) {
-idedev = pci_ide->bus[di->bus].master;
+idedev = idebus->master;
} else {
-idedev = pci_ide->bus[di->bus].slave;
+idedev = idebus->slave;
}
+
+blk_drain(blk);
+blk_flush(blk);
+
+blk_detach_dev(blk, DEVICE(idedev));
+idebus->ifs[i % 2].blk = NULL;
idedev->conf.blk = NULL;
monitor_remove_blk(blk);
blk_unref(blk);
--
Anthony PERARD
[PATCH] xen: rework pci_piix3_xen_ide_unplug
This is to allow IDE disks to be unplugged when adding to QEMU via:
-drive file=/root/disk_file,if=none,id=ide-disk0,format=raw
-device ide-hd,drive=ide-disk0,bus=ide.0,unit=0
as the current code only works for disk added with:
-drive file=/root/disk_file,if=ide,index=0,media=disk,format=raw
Since the code already have the IDE controller as `dev`, we don't need
to use the legacy DriveInfo to find all the drive we want to unplug.
We can simply use `blk` from the controller, as it kind of was already
assume to be the same, by setting it to NULL.
Signed-off-by: Anthony PERARD
---
hw/ide/piix.c | 27 +--
1 file changed, 13 insertions(+), 14 deletions(-)
diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index b402a936362b..16fcbe58d6fe 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -164,30 +164,29 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error
**errp)
int pci_piix3_xen_ide_unplug(DeviceState *dev, bool aux)
{
PCIIDEState *pci_ide;
-DriveInfo *di;
int i;
IDEDevice *idedev;
+IDEBus *idebus;
+BlockBackend *blk;
pci_ide = PCI_IDE(dev);
for (i = aux ? 1 : 0; i < 4; i++) {
-di = drive_get_by_index(IF_IDE, i);
-if (di != NULL && !di->media_cd) {
-BlockBackend *blk = blk_by_legacy_dinfo(di);
-DeviceState *ds = blk_get_attached_dev(blk);
+idebus = &pci_ide->bus[i / 2];
+blk = idebus->ifs[i % 2].blk;
-blk_drain(blk);
-blk_flush(blk);
-
-if (ds) {
-blk_detach_dev(blk, ds);
-}
-pci_ide->bus[di->bus].ifs[di->unit].blk = NULL;
+if (blk && idebus->ifs[i%2].drive_kind != IDE_CD) {
if (!(i % 2)) {
-idedev = pci_ide->bus[di->bus].master;
+idedev = idebus->master;
} else {
-idedev = pci_ide->bus[di->bus].slave;
+idedev = idebus->slave;
}
+
+blk_drain(blk);
+blk_flush(blk);
+
+blk_detach_dev(blk, DEVICE(idedev));
+idebus->ifs[i % 2].blk = NULL;
idedev->conf.blk = NULL;
monitor_remove_blk(blk);
blk_unref(blk);
--
Anthony PERARD
