Luiz Capitulino <lcapitul...@redhat.com> 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 <lcapitul...@redhat.com>
> ---
> 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.
