Re: [Qemu-devel] [PATCH 3/5] raw-posix: keep complete control of door locking if possible

[Markus Armbruster]

Paolo Bonzini <address@hidden> writes:

> Try to open the disk O_EXCL so that udev will not receive eject
> request and media change events.  These will work fine with SCSI
> passthrough.
>
> With IDE and scsi-disk the user will need to use the monitor in order
> to eject the disk and put it back (respectively with the eject and
> change commands).  Fixing this would require (re)implementing polling
> using CDROM_MEDIA_CHANGED ioctls.

You mean the physical eject button doesn't work with IDE and scsi-disk,
don't you?  If yes, please state that in the commit message.

If I read your patch correctly, you're actually implementing two modes:
manage_door on and off.

You get manage_door on iff we can open the device O_EXCL.

With manage_door off:

* cdrom_lock_medium() does nothing.  Thus, guest OS operating the
  virtual door lock does not affect the physical door.

* cdrom_eject() does nothing.  Thus, guest OS moving the virtual tray
  does not affect the physical tray.

Compare to before this patch:

* cdrom_lock_medium() fails silently.  Thus, guest OS operating the
  virtual door may or may not affect the physical door.  It usually does
  unless the CD-ROM is mounted in the host.

* cdrom_eject() perror()s when it fails.  Thus, guest OS moving the
  virtual tray may or may not affect the physocal tray.  It usually does
  unless the CD-ROM is mounted in the host, or udev got its paws on it
  (I think).

With manage_door on:

* cdrom_lock_medium() works exactly as before, except the common failure
  mode "CD-ROM is mounted in the host" can't happen.

* cdrom_eject() works exactly as before, except the common failure mode
  "CD-ROM is mounted in the host" can't happen.

Is this correct?

If yes, is it what we want?  Shouldn't the user be able to ask for one
of "grab the CD-ROM exclusively, else fail", "grab it if you can",
"don't grab it even if you could"?

Regardless, describing the two modes in the commit message would be
nice.

> Signed-off-by: Paolo Bonzini <address@hidden>
> ---
>  block/raw-posix.c |   89 +++++++++++++++++++++++++++++++++++++++++++++++-----
>  1 files changed, 80 insertions(+), 9 deletions(-)
>
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index d9b03a1..1b51bd4 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -127,6 +127,11 @@ typedef struct BDRVRawState {
>              int got_error;
>              int media_changed;
>          } fdd;
> +     struct {
> +            bool manage_door;
> +            bool save_lock;
> +            bool save_locked;
> +     } cd;

Only used under Linux, not under FreeBSD, suggest a comment.  Do you
think the FreeBSD implementation of host_cdrom could use this, too, or
is it simply Linux-specific?

>      };
>  #endif
>  #ifdef CONFIG_LINUX_AIO
> @@ -1035,14 +1040,80 @@ static BlockDriver bdrv_host_floppy = {
>      .bdrv_eject         = floppy_eject,
>  };
>  
> +static bool cdrom_get_options(int fd)

Shouldn't this return int?

> +{
> +    /* CDROM_SET_OPTIONS with arg == 0 returns the current options!  */
> +    return ioctl(fd, CDROM_SET_OPTIONS, 0);
> +}
> +
> +static int cdrom_is_locked(int fd)
> +{
> +    int opts = cdrom_get_options(fd);
> +
> +    /* This is the only way we have to probe the current status of
> +     * CDROM_LOCKDOOR (EBUSY = locked).  We use CDROM_SET_OPTIONS to
> +     * reset the previous state in case the ioctl succeeds.
> +     */
> +    if (ioctl(fd, CDROMEJECT_SW, 0) == 0) {
> +        ioctl(fd, CDROM_SET_OPTIONS, opts);
> +        return false;
> +    } else if (errno == EBUSY) {
> +        return true;
> +    } else {
> +        return -errno;
> +    }
> +}
> +
> +
>  static int cdrom_open(BlockDriverState *bs, const char *filename, int flags)
>  {
>      BDRVRawState *s = bs->opaque;
> +    int rc;
>  
>      s->type = FTYPE_CD;
>  
> -    /* open will not fail even if no CD is inserted, so add O_NONBLOCK */
> -    return raw_open_common(bs, filename, flags, O_NONBLOCK);
> +    /* open will not fail even if no CD is inserted, so add O_NONBLOCK.  
> First

Long line, please wrap.

> +     * try with O_EXCL to see whether the CD is mounted.
> +     */
> +    rc = raw_open_common(bs, filename, flags, O_NONBLOCK | O_EXCL);
> +    if (rc < 0 && rc != -EBUSY) {
> +        return rc;
> +    }
> +
> +    s->cd.manage_door = false;
> +    if (rc == -EBUSY) {
> +        /* The CD-ROM is mounted.  No door support, because if we cannot use
> +         * O_EXCL udev will always succeed in ejecting the medium under our
> +         * feet.
> +         */
> +        rc = raw_open_common(bs, filename, flags, O_NONBLOCK);
> +    } else if (rc == 0) {
> +        /* We can handle the door ourselves.  Save state so we can restore
> +         * it when we exit.
> +         */
> +        int is_locked = cdrom_is_locked(s->fd);
> +        if (is_locked >= 0 && ioctl(s->fd, CDROM_LOCKDOOR, 0) != -1) {
> +            s->cd.save_lock = (cdrom_get_options(s->fd) & CDO_LOCK) != 0;
> +            s->cd.save_locked = is_locked;
> +            s->cd.manage_door = true;
> +
> +            ioctl(s->fd, CDROM_CLEAR_OPTIONS, CDO_LOCK);
> +        }
> +    }
> +
> +    return rc;
> +}
> +
> +static void cdrom_close(BlockDriverState *bs)
> +{
> +    BDRVRawState *s = bs->opaque;
> +    if (s->fd >= 0 && s->cd.manage_door) {
> +        ioctl(s->fd, CDROM_LOCKDOOR, s->cd.save_locked);
> +        if (s->cd.save_lock) {
> +            ioctl(s->fd, CDROM_SET_OPTIONS, CDO_LOCK);
> +        }
> +    }
> +    raw_close(bs);
>  }
>  
>  static int cdrom_probe_device(const char *filename)
> @@ -1086,6 +1157,10 @@ static void cdrom_eject(BlockDriverState *bs, int 
> eject_flag)
>  {
>      BDRVRawState *s = bs->opaque;
>  
> +    if (!s->cd.manage_door) {
> +        return;
> +    }
> +
>      if (eject_flag) {
>          if (ioctl(s->fd, CDROMEJECT, NULL) < 0)
>              perror("CDROMEJECT");

We report ioctl() failing here...

> @@ -1099,12 +1174,8 @@ static void cdrom_lock_medium(BlockDriverState *bs, 
> bool locked)
>  {
>      BDRVRawState *s = bs->opaque;
>  
> -    if (ioctl(s->fd, CDROM_LOCKDOOR, locked) < 0) {
> -        /*
> -         * Note: an error can happen if the distribution automatically
> -         * mounts the CD-ROM
> -         */
> -        /* perror("CDROM_LOCKDOOR"); */
> +    if (s->cd.manage_door) {
> +        ioctl(s->fd, CDROM_LOCKDOOR, locked);
>      }
>  }

... but not here.  Any particular reason for treating the two
differently?

>  
> @@ -1114,7 +1185,7 @@ static BlockDriver bdrv_host_cdrom = {
>      .instance_size      = sizeof(BDRVRawState),
>      .bdrv_probe_device       = cdrom_probe_device,
>      .bdrv_file_open     = cdrom_open,
> -    .bdrv_close         = raw_close,
> +    .bdrv_close         = cdrom_close,
>      .bdrv_create        = hdev_create,
>      .create_options     = raw_create_options,
>      .bdrv_has_zero_init = hdev_has_zero_init,