Re: [Qemu-devel] [PATCH] Prevent CD-ROM media eject while device is locked

[Kevin Wolf]

Mark McLoughlin schrieb:
> Section 10.8.25 ("START/STOP UNIT Command") of SFF-8020i states that
> if the device is locked we should refuse to eject if the device is
> locked.
> 
> ASC_MEDIA_REMOVAL_PREVENTED is the appropriate return in this case.
> 
> In order to stop itself from ejecting the media it is running from,
> Fedora's installer (anaconda) requires the CDROMEJECT ioctl() to fail
> if the drive has been previously locked.
> 
> See also https://bugzilla.redhat.com/501412
> 
> Signed-off-by: Mark McLoughlin <address@hidden>
> ---
>  block.c  |    9 ++++++++-
>  block.h  |    2 +-
>  hw/ide.c |   26 ++++++++++++++++++--------
>  3 files changed, 27 insertions(+), 10 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 9a2873f..863897a 100644
> --- a/block.c
> +++ b/block.c
> @@ -1673,11 +1673,15 @@ int bdrv_media_changed(BlockDriverState *bs)
>  /**
>   * If eject_flag is TRUE, eject the media. Otherwise, close the tray
>   */
> -void bdrv_eject(BlockDriverState *bs, int eject_flag)
> +int bdrv_eject(BlockDriverState *bs, int eject_flag)
>  {
>      BlockDriver *drv = bs->drv;
>      int ret;
>  
> +    if (bs->locked) {
> +        return -EBUSY;
> +    }
> +
>      if (!drv || !drv->bdrv_eject) {
>          ret = -ENOTSUP;
>      } else {
> @@ -1686,7 +1690,10 @@ void bdrv_eject(BlockDriverState *bs, int eject_flag)
>      if (ret == -ENOTSUP) {
>          if (eject_flag)
>              bdrv_close(bs);
> +        ret = 0;
>      }
> +
> +    return ret;
>  }
>  
>  int bdrv_is_locked(BlockDriverState *bs)
> diff --git a/block.h b/block.h
> index 979781a..e1070e9 100644
> --- a/block.h
> +++ b/block.h
> @@ -132,7 +132,7 @@ int bdrv_is_inserted(BlockDriverState *bs);
>  int bdrv_media_changed(BlockDriverState *bs);
>  int bdrv_is_locked(BlockDriverState *bs);
>  void bdrv_set_locked(BlockDriverState *bs, int locked);
> -void bdrv_eject(BlockDriverState *bs, int eject_flag);
> +int bdrv_eject(BlockDriverState *bs, int eject_flag);
>  void bdrv_set_change_cb(BlockDriverState *bs,
>                          void (*change_cb)(void *opaque), void *opaque);
>  void bdrv_get_format(BlockDriverState *bs, char *buf, int buf_size);
> diff --git a/hw/ide.c b/hw/ide.c
> index 6ad1d08..9b93e7f 100644
> --- a/hw/ide.c
> +++ b/hw/ide.c
> @@ -359,6 +359,7 @@
>  #define ASC_INCOMPATIBLE_FORMAT              0x30
>  #define ASC_MEDIUM_NOT_PRESENT               0x3a
>  #define ASC_SAVING_PARAMETERS_NOT_SUPPORTED  0x39
> +#define ASC_MEDIA_REMOVAL_PREVENTED          0x53
>  
>  #define CFA_NO_ERROR            0x00
>  #define CFA_MISC_ERROR          0x09
> @@ -1818,18 +1819,27 @@ static void ide_atapi_cmd(IDEState *s)
>          break;
>      case GPCMD_START_STOP_UNIT:
>          {
> -            int start, eject;
> +            int start, eject, err = 0;
>              start = packet[4] & 1;
>              eject = (packet[4] >> 1) & 1;
>  
> -            if (eject && !start) {
> -                /* eject the disk */
> -                bdrv_eject(s->bs, 1);
> -            } else if (eject && start) {
> -                /* close the tray */
> -                bdrv_eject(s->bs, 0);
> +            if (eject) {
> +                err = bdrv_eject(s->bs, !start);
> +            }

I would actually prefer to retain the comments on what start means in
the eject case. Otherwise looks good to me.

hw/scsi-disk.c contains another reference to bdrv_eject. Wouldn't it
make sense to check the return value there, too?

Kevin