Re: [Qemu-devel] [RFC for-1.4] Revert "block: fix block tray status"

[Kevin Wolf]

Am 06.02.2013 21:02, schrieb Luiz Capitulino:
> This reverts commit 9ca111544c64b5abed2e79cf52e19a8f227b347b.
> 
> That commit has added a weird side effect to QMP: on shutdown,
> QMP emits the DEVICE_TRAY_MOVED event for all empty drives
> that have closed trays.
> 
> This happens because the tray state actually changes in
> the following code path:
> 
>   main loop exit
>     bdrv_close_all()
>      bdrv_close()
>       bdrv_dev_change_media_cb()
> 
> So, revert the commit as the real fix is to separate eject
> from bdrv_close() and also because it's not clear how important
> it is to have a 100% correct semantic for eject from HMP.
> 
> Signed-off-by: Luiz Capitulino <address@hidden>
> ---
> 
> ** IMPORTANT: This is an RFC because I'm not totally sure we should
> apply it for 1.4, for the following reasons:
> 
>  1. The worst we get is a gratuitous state change that triggers
>     an QMP event
> 
>  2. Commit 9ca1115 is a bit old and was introduced before 1.4
>     development
> 
>  3. This is not the Right Thing either
> 
>  block.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 50dab8e..d58c6cc 100644
> --- a/block.c
> +++ b/block.c
> @@ -1178,9 +1178,9 @@ void bdrv_close(BlockDriverState *bs)
>              bdrv_delete(bs->file);
>              bs->file = NULL;
>          }
> -    }
>  
> -    bdrv_dev_change_media_cb(bs, false);
> +        bdrv_dev_change_media_cb(bs, false);
> +    }
>  
>      /*throttling disk I/O limits*/
>      if (bs->io_limits_enabled) {

I think the right solution is to move the bdrv_dev_change_media_cb()
call to those callers of bdrv_close() that actually need it. I haven't
checked it yet in detail, but at the first sight it looks like
eject_device() and do_drive_del() are the only ones that need it.

Such a change I'd rather move to 1.5, though. Do you think it's
important to get rid of these (admittedly odd) artefacts for 1.4?

Kevin