Re: [Qemu-devel] [PATCH 6/8] block: add eject request callback

[Markus Armbruster <armbru@redhat.com>]

I apologize for the lateness of this review.

Paolo Bonzini <pbonzini@redhat.com> writes:

> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

The commit message should explain why we need this callback.  The cover
letter says "support for eject requests is required by udev 173."
Please elaborate on that.

> ---
>  block.c    |    7 +++++++
>  block.h    |    7 +++++++
>  blockdev.c |    8 +++++---
>  3 files changed, 19 insertions(+), 3 deletions(-)
>
> diff --git a/block.c b/block.c
> index 9873b57..53e21ba 100644
> --- a/block.c
> +++ b/block.c
> @@ -821,6 +821,13 @@ bool bdrv_dev_has_removable_media(BlockDriverState *bs)
>      return !bs->dev || (bs->dev_ops && bs->dev_ops->change_media_cb);
>  }
>  
> +void bdrv_dev_eject_request(BlockDriverState *bs, bool force)
> +{
> +    if (bs->dev_ops && bs->dev_ops->eject_request_cb) {
> +        bs->dev_ops->eject_request_cb(bs->dev_opaque, force);
> +    }
> +}
> +
>  bool bdrv_dev_is_tray_open(BlockDriverState *bs)
>  {
>      if (bs->dev_ops && bs->dev_ops->is_tray_open) {
> diff --git a/block.h b/block.h
> index e77988e..d3c3d62 100644
> --- a/block.h
> +++ b/block.h
> @@ -39,6 +39,12 @@ typedef struct BlockDevOps {
>       */
>      void (*change_media_cb)(void *opaque, bool load);
>      /*
> +     * Runs when an eject request is issued from the monitor, the tray
> +     * is closed, and the medium is locked.
> +     * Device models with removable media must implement this callback.
> +     */
> +    void (*eject_request_cb)(void *opaque, bool force);
> +    /*

You implement it for IDE in PATCH 7/8 and SCSI in PATCH 8/8.  You don't
implement it for floppy, despite the comment.  That's okay; floppy has
no use for it.  It's the comment that needs fixing.  Devices that
implement is_medium_locked() must implement this one as well.

>       * Is the virtual tray open?
>       * Device models implement this only when the device has a tray.
>       */
> @@ -116,6 +122,7 @@ void bdrv_detach_dev(BlockDriverState *bs, void *dev);
>  void *bdrv_get_attached_dev(BlockDriverState *bs);
>  void bdrv_set_dev_ops(BlockDriverState *bs, const BlockDevOps *ops,
>                        void *opaque);
> +void bdrv_dev_eject_request(BlockDriverState *bs, bool force);
>  bool bdrv_dev_has_removable_media(BlockDriverState *bs);
>  bool bdrv_dev_is_tray_open(BlockDriverState *bs);
>  bool bdrv_dev_is_medium_locked(BlockDriverState *bs);
> diff --git a/blockdev.c b/blockdev.c
> index 0827bf7..4cf333a 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -635,9 +635,11 @@ static int eject_device(Monitor *mon, BlockDriverState *bs, int force)
>          qerror_report(QERR_DEVICE_NOT_REMOVABLE, bdrv_get_device_name(bs));
>          return -1;
>      }
> -    if (!force && !bdrv_dev_is_tray_open(bs)
> -        && bdrv_dev_is_medium_locked(bs)) {
> -        qerror_report(QERR_DEVICE_LOCKED, bdrv_get_device_name(bs));
> +    if (bdrv_dev_is_medium_locked(bs) && !bdrv_dev_is_tray_open(bs)) {
> +        bdrv_dev_eject_request(bs, force);
> +        if (!force) {
> +            qerror_report(QERR_DEVICE_LOCKED, bdrv_get_device_name(bs));
> +        }
>          return -1;
>      }
>      bdrv_close(bs);

Like Kevin, I'm not entirely comfortable with changing the meaning of
"-f".

Here's my mental model of monitor command eject:

1. eject without -f behaves like the physical tray button.  It has
immediate effect, unless the tray is locked closed.  Then, the drive
just notifies the OS of the button push, so the OS can react to it.  The
latter isn't implemented in QEMU.

2. eject with -f behaves like whatever physical way there is to pry the
tray open, locked or not.  CD-ROM drives commonly have a little button
hidden in some hope you can reach with a bent paperclip.

Could you explain your mental model?