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

[Paolo Bonzini]

On 02/10/2012 01:49 PM, Markus Armbruster wrote:
> 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.

That's because manage_door off means "we couldn't get O_EXCL to work". 
udev will affect the physical tray and we cannot really compete with it.

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

For virtio-scsi+scsi-block, we definitely want O_EXCL to remove the 
device from udev and the in-kernel poller.  That's because GESN reports 
events only once, and we do not want the host and guest to race on 
receiving events.  But in order to open with O_EXCL we need to unmount 
(GNOME3 doesn't have an unmount menu item anymore, which is why I did it 
myself in patch 5/5).

For IDE we do not need O_EXCL in principle.  However, using the emulated 
passthrough CD-ROM gets confusing without O_EXCL (to the user and to the 
guest, always; and sometimes, to the host udev as well).  Basically, the 
guest tries to lock the disk, but udev runs as root so it can unlock it 
under its feet as soon as you press the button.  With O_EXCL and some 
extra patches (waiting for Luiz's eject event discussion), everything 
kinda works; still confusing to the user, but not to the host or VM.

I have some prototype patches to improve the situation on IDE somehow 
(still a far cry from scsi-block), but I'm waiting for Luiz's eject 
event patches to reach a conclusion first.

There is also the case of emulated passthrough SCSI CD-ROM, but I don't 
care much about it.

> 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"?

Perhaps IDE could be made to work cooperatively, but right now it 
doesn't.  But scsi-block should fail if it cannot manage the door 
because it bypasses all the CD-ROM machinery and issues SCSI commands 
directly.

> > +static bool cdrom_get_options(int fd)
>
> Shouldn't this return int?

Yes. *paper-bag*

> > +{
> > +    /* 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.

I count 78 chars. :)

> > @@ -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?

Not really, I was just keeping the existing code, but the comment about 
automatic mount does not hold anymore since we have O_EXCL.  I think not 
reporting any error should work just as well.

Paolo