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

[Luiz Capitulino]

On Thu, 25 Apr 2013 20:18:35 +0200
Markus Armbruster <address@hidden> wrote:

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

Very encouraging :( I wonder if such a comprehensive analysis was done
when 1 adding the callback in the first place and 2 on 9ca1115.

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

The bug I'm fixing is not a serious one, but this makes me think that
reverting 9ca1115 is the best thing to do.