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

[Luiz Capitulino]

On Thu, 07 Feb 2013 14:12:10 +0100
Kevin Wolf <address@hidden> wrote:

> 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?

I don't have any reports of this causing issues to QMP clients, so
we can defer to 1.5. Worst case we merge this fix for a stable release.