Re: [Qemu-devel] [PATCH 2/2] block: move bdrv_dev_change_media_cb() to callers that really need it

[Markus Armbruster <armbru@redhat.com>]

Luiz Capitulino <lcapitulino@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 <lcapitulino@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.