On Thu, 07 Feb 2013 14:12:10 +0100
Kevin Wolf <kw...@redhat.com> 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 <lcapitul...@redhat.com>
> > ---
> >
> > ** 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.
