Luiz Capitulino <address@hidden> writes:
Commit 9ca111544c64b5abed2e79cf52e19a8f227b347b moved the call to
bdrv_dev_change_media_cb() outside the media check in bdrv_close(),
this added a regression where spurious DEVICE_TRAY_MOVED events
are emitted at shutdown.
To fix that this commit moves the bdrv_dev_change_media_cb() calls
to the callers that really need to report a media change, which
are eject_device() and do_drive_del(). This fixes the problem
commit 9ca1115 intended to fix, plus the spurious events.
Signed-off-by: Luiz Capitulino <address@hidden>
---
block.c | 2 --
blockdev.c | 2 ++
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/block.c b/block.c
index 90d0ed1..7fc3014 100644
--- a/block.c
+++ b/block.c
@@ -1342,8 +1342,6 @@ void bdrv_close(BlockDriverState *bs)
}
}
- bdrv_dev_change_media_cb(bs, false);
-
/*throttling disk I/O limits*/
if (bs->io_limits_enabled) {
bdrv_io_limits_disable(bs);
diff --git a/blockdev.c b/blockdev.c
index 8a1652b..f1f3b6e 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -950,6 +950,7 @@ static void eject_device(BlockDriverState *bs, int force,
Error **errp)
}
bdrv_close(bs);
+ bdrv_dev_change_media_cb(bs, false);
}
void qmp_eject(const char *device, bool has_force, bool force, Error **errp)
@@ -1100,6 +1101,7 @@ int do_drive_del(Monitor *mon, const QDict *qdict,
QObject **ret_data)
bdrv_drain_all();
bdrv_flush(bs);
bdrv_close(bs);
+ bdrv_dev_change_media_cb(bs, false);
/* if we have a device attached to this BlockDriverState
* then we need to make the drive anonymous until the device
Invariant: callback does nothing unless a device model with removable
media is connected (dev_ops->change_media_cb set).
Before 9ca1115: Callback runs on any bdrv_close() that actually ejects a
medium.
Since 9ca1115: Callback runs on any bdrv_close()
With this patch applied: Callback runs in eject_device() and
do_drive_del(). No change, except it now runs after
bdrv_io_limits_disable(), which shouldn't matter. This is the trivial
part of the review.
Now the non-trivial part. Callback no longer runs in
* bdrv_open() when it fails because it can't open the backing file
* bdrv_open() when it fails because the block driver doesn't consume the
all options
* bdrv_delete()
- bdrv_file_open() error path
- bdrv_open_backing_file() error path
- bdrv_open() snapshot=on path
- bdrv_open(), purpose not obvious, perhaps related to format probing
- bdrv_open() some error paths
- bdrv_close(), bs->backing_hd
- bdrv_close(), bs->file
- bdrv_drop_intermediate()
- bdrv_snapshot_goto() error path
- bdrv_img_create()
- drive_uninit()
- drive_init() error path
- qmp_transaction() error path
- qmp_drive_mirror() some error paths
- qemu_img.c many places
- qemu_io.c many places
- blkverify_open() error path
- blkverify_close()
- cow_create()
- mirror_run()
- qcow_create()
- qcow2_create2()
- qed_create()
- sheepdog.c's sd_prealloc(), sd_create()
- close_unused_images()
- vmdk_free_extents(), vmdk_parse_extents(), vmdk_create()
- vvfat.c's write_target_close()
* qemu-nbd.c's main()
* mirror_run()
* qcow2_create2()
* pci_piix3_xen_ide_unplug()
* bdrv_close_all()
- qemu-nbd.c, from atexit() Same as above.
- vl.c's main().
For each of them, I'd like to see an argument why the not running the
callback is okay. A good one is "no device model can be attached", say
because the BDS isn't a root (device models only attach to roots), or
because the BDS hasn't escaped its constructor, yet.
Not wanting to do all this work is exactly why I refrained from
attempting to fix the problem commit 9ca1115 attempts to fix (I
suggested to declare it a feature instead). Didn't help, because I also
refrained from NAKing that fix, and here we are.
I'll do what I can, as time permits, but help is certainly appreciated.