[Qemu-devel] [PATCH 0/5] Fix CD-ROM door with SCSI passthrough

[Qemu-devel] [PATCH 0/5] Fix CD-ROM door with SCSI passthrough

[Paolo Bonzini]

This series makes it possible to use CD drives reliably with scsi-block.
IDE and scsi-disk require more work, because the eject button is not
usable.

CD drives are unmounted at VM start and opened exclusively.  The locking
state is saved and restored for extra kindness.  However, drives are not
remounted because they are unlikely to be needed in the host (they were
CDs for a guest after all), and repeated close/open done by the block
layer would turn into unreliable mount/unmount requests to udisks
(unmounts after the first would return EBUSY).

Paolo Bonzini (5):
  raw-posix: always prefer specific devices to hdev
  raw-posix: put Linux fd fields into a union
  raw-posix: keep complete control of door locking if possible
  configure: probe for dbus
  raw-posix: unmount CD-ROM filesystem via udisks

 Makefile.objs            |    3 +
 block.c                  |    7 ++
 block/raw-posix-udisks.c |  105 ++++++++++++++++++++++++++++++++
 block/raw-posix-udisks.h |   39 ++++++++++++
 block/raw-posix.c        |  150 +++++++++++++++++++++++++++++++++++-----------
 configure                |   39 ++++++++++++
 6 files changed, 308 insertions(+), 35 deletions(-)
 create mode 100644 block/raw-posix-udisks.c
 create mode 100644 block/raw-posix-udisks.h

-- 
1.7.7.6

[Qemu-devel] [PATCH 1/5] raw-posix: always prefer specific devices to hdev

[Paolo Bonzini]

There is no need to try matching device names; using ioctls is more
effective.  So, always return a low priority from the generic
hdev_probe_device and let the ioctl tests override it.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/raw-posix.c |   12 +++---------
 1 files changed, 3 insertions(+), 9 deletions(-)

diff --git a/block/raw-posix.c b/block/raw-posix.c
index 2ee5d69..2a5b6fa 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -712,13 +712,9 @@ static int hdev_probe_device(const char *filename)
 {
     struct stat st;
 
-    /* allow a dedicated CD-ROM driver to match with a higher priority */
-    if (strstart(filename, "/dev/cdrom", NULL))
-        return 50;
-
     if (stat(filename, &st) >= 0 &&
             (S_ISCHR(st.st_mode) || S_ISBLK(st.st_mode))) {
-        return 100;
+        return 50;
     }
 
     return 0;
@@ -947,9 +943,6 @@ static int floppy_probe_device(const char *filename)
     struct floppy_struct fdparam;
     struct stat st;
 
-    if (strstart(filename, "/dev/fd", NULL))
-        prio = 50;
-
     fd = open(filename, O_RDONLY | O_NONBLOCK);
     if (fd < 0) {
         goto out;
@@ -959,7 +952,8 @@ static int floppy_probe_device(const char *filename)
         goto outc;
     }
 
-    /* Attempt to detect via a floppy specific ioctl */
+    /* Attempt to detect via a floppy specific ioctl.  If it fails,
+     * hdev will be just as good.  */
     ret = ioctl(fd, FDGETPRM, &fdparam);
     if (ret >= 0)
         prio = 100;
-- 
1.7.7.6

Re: [Qemu-devel] [PATCH 1/5] raw-posix: always prefer specific devices to hdev

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> There is no need to try matching device names; using ioctls is more
> effective.  So, always return a low priority from the generic
> hdev_probe_device and let the ioctl tests override it.

Matching file names is lame.  There are a few instances left, but this
is a welcome improvement.

[Qemu-devel] [PATCH 2/5] raw-posix: put Linux fd fields into a union

[Paolo Bonzini]

We will add CDROM-specific fields in the next patch, reuse the space
in the struct.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/raw-posix.c |   38 +++++++++++++++++++++-----------------
 1 files changed, 21 insertions(+), 17 deletions(-)

diff --git a/block/raw-posix.c b/block/raw-posix.c
index 2a5b6fa..d9b03a1 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -119,11 +119,15 @@ typedef struct BDRVRawState {
     int type;
     int open_flags;
 #if defined(__linux__)
-    /* linux floppy specific */
-    int64_t fd_open_time;
-    int64_t fd_error_time;
-    int fd_got_error;
-    int fd_media_changed;
+    union {
+        struct {
+            /* linux floppy specific */
+            int64_t open_time;
+            int64_t error_time;
+            int got_error;
+            int media_changed;
+        } fdd;
+    };
 #endif
 #ifdef CONFIG_LINUX_AIO
     int use_aio;
@@ -779,7 +783,7 @@ static int fd_open(BlockDriverState *bs)
         return 0;
     last_media_present = (s->fd >= 0);
     if (s->fd >= 0 &&
-        (get_clock() - s->fd_open_time) >= FD_OPEN_TIMEOUT) {
+        (get_clock() - s->fdd.open_time) >= FD_OPEN_TIMEOUT) {
         close(s->fd);
         s->fd = -1;
 #ifdef DEBUG_FLOPPY
@@ -787,8 +791,8 @@ static int fd_open(BlockDriverState *bs)
 #endif
     }
     if (s->fd < 0) {
-        if (s->fd_got_error &&
-            (get_clock() - s->fd_error_time) < FD_OPEN_TIMEOUT) {
+        if (s->fdd.got_error &&
+            (get_clock() - s->fdd.error_time) < FD_OPEN_TIMEOUT) {
 #ifdef DEBUG_FLOPPY
             printf("No floppy (open delayed)\n");
 #endif
@@ -796,10 +800,10 @@ static int fd_open(BlockDriverState *bs)
         }
         s->fd = open(bs->filename, s->open_flags & ~O_NONBLOCK);
         if (s->fd < 0) {
-            s->fd_error_time = get_clock();
-            s->fd_got_error = 1;
+            s->fdd.error_time = get_clock();
+            s->fdd.got_error = 1;
             if (last_media_present)
-                s->fd_media_changed = 1;
+                s->fdd.media_changed = 1;
 #ifdef DEBUG_FLOPPY
             printf("No floppy\n");
 #endif
@@ -810,9 +814,9 @@ static int fd_open(BlockDriverState *bs)
 #endif
     }
     if (!last_media_present)
-        s->fd_media_changed = 1;
-    s->fd_open_time = get_clock();
-    s->fd_got_error = 0;
+        s->fdd.media_changed = 1;
+    s->fdd.open_time = get_clock();
+    s->fdd.got_error = 0;
     return 0;
 }
 
@@ -931,7 +935,7 @@ static int floppy_open(BlockDriverState *bs, const char *filename, int flags)
     /* close fd so that we can reopen it as needed */
     close(s->fd);
     s->fd = -1;
-    s->fd_media_changed = 1;
+    s->fdd.media_changed = 1;
 
     return 0;
 }
@@ -980,8 +984,8 @@ static int floppy_media_changed(BlockDriverState *bs)
      * It does not work if the floppy is changed without trying to read it.
      */
     fd_open(bs);
-    ret = s->fd_media_changed;
-    s->fd_media_changed = 0;
+    ret = s->fdd.media_changed;
+    s->fdd.media_changed = 0;
 #ifdef DEBUG_FLOPPY
     printf("Floppy changed=%d\n", ret);
 #endif
-- 
1.7.7.6

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

[Paolo Bonzini]

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.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 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;
     };
 #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)
+{
+    /* 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
+     * 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");
@@ -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);
     }
 }
 
@@ -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,
-- 
1.7.7.6

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

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> 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 <pbonzini@redhat.com>
> ---
>  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,

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

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

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

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

Understood.  I'm just describing behavior to make sure I understand your
patch, not criticizing 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).

Program A unmounting a filesystem that some unknown program B mounted
for its own unknown reasons doesn't feel right to me.  Even when program
B sucks as badly as Gnome appears to do.  Especially when program A does
it automatically, silently, and without a way to switch it off.

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

You've made a good case for why we really, really want managed_door on.
With managed_door off, both software and humans basically run around
confused.

Why do we even offer managed_door off then?

And isn't the automatic, *silent* fallback to the rotten user experience
of managed_door off a recipe for support calls?

[...]

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

[Paolo Bonzini]

On 02/10/2012 03:56 PM, Markus Armbruster wrote:
> Paolo Bonzini <pbonzini@redhat.com> writes:
>
>> 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.
>
> Understood.  I'm just describing behavior to make sure I understand your
> patch, not criticizing it.

Sure, just expanding on the rationale.

>> 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).
>
> Program A unmounting a filesystem that some unknown program B mounted
> for its own unknown reasons doesn't feel right to me.  Even when program
> B sucks as badly as Gnome appears to do.  Especially when program A does
> it automatically, silently, and without a way to switch it off.

Well, first it's a CD-ROM, so it won't cause data loss.  CD burning 
programs use O_EXCL so QEMU will not stomp on anyone's feet while the CD 
is being written to.

Second, it's the user who asked to use the CD-ROM in a guest.  It makes 
sense to imply he does not want to use it in the host.

Third, if there are open files or such, the unmounting will fail.

And in fact in most cases you won't notice the auto unmounting.  Say 
you're installing a guest (most common case of IDE CD passthrough).  The 
installation program will usually eject the disc before rebooting.  If 
you put it back in after you stop the VM and QEMU has exited, ta-dah, 
the disc will be remounted.

> You've made a good case for why we really, really want managed_door on.
> With managed_door off, both software and humans basically run around
> confused.

Kind of.  In most cases you don't care at all about the tray (for 
example when installing a VM from a "real" disk).  You care about the 
tray if the guest starts using it, for example Anaconda's infamous 
auto-eject after testing media.  Right now sometimes it's hard to 
convince the guest that you actually put the CD back.  But if the tray 
is not managed, you'll never get an auto-eject and you don't have to 
convince anyone about anything.

> Why do we even offer managed_door off then?
> And isn't the automatic, *silent* fallback to the rotten user experience
> of managed_door off a recipe for support calls?

Regarding silence you have a point.  On the other hand, when starting 
via libvirt everything will be silent. :(  This is another point in 
favor of automatic unmounting, which will usually just work.

It may make sense to fail completely if the unmounting fails, but then 
we need to do more effort to grab the disk (e.g. try /bin/umount if 
udisks fails).

Paolo

[Qemu-devel] [PATCH 4/5] configure: probe for dbus

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 configure |   39 +++++++++++++++++++++++++++++++++++++++
 1 files changed, 39 insertions(+), 0 deletions(-)

diff --git a/configure b/configure
index 763db24..0f305cb 100755
--- a/configure
+++ b/configure
@@ -119,6 +119,7 @@ curl=""
 curses=""
 docs=""
 fdt=""
+dbus=""
 nptl=""
 sdl=""
 vnc="yes"
@@ -510,6 +511,7 @@ if test "$mingw32" = "yes" ; then
   sysconfdir="\${prefix}"
   confsuffix=""
   guest_agent="no"
+  dbus="no"
 fi
 
 werror=""
@@ -623,6 +625,10 @@ for opt do
   ;;
   --disable-strip) strip_opt="no"
   ;;
+  --disable-dbus) dbus="no"
+  ;;
+  --enable-dbus) dbus="yes"
+  ;;
   --disable-vnc-tls) vnc_tls="no"
   ;;
   --enable-vnc-tls) vnc_tls="yes"
@@ -1007,6 +1013,8 @@ echo "  --disable-xen            disable xen backend driver support"
 echo "  --enable-xen             enable xen backend driver support"
 echo "  --disable-brlapi         disable BrlAPI"
 echo "  --enable-brlapi          enable BrlAPI"
+echo "  --disable-dbus           disable dbus interaction"
+echo "  --enable-dbus            enable dbus interaction"
 echo "  --disable-vnc-tls        disable TLS encryption for VNC server"
 echo "  --enable-vnc-tls         enable TLS encryption for VNC server"
 echo "  --disable-vnc-sasl       disable SASL encryption for VNC server"
@@ -1987,6 +1995,32 @@ else
 fi
 
 ##########################################
+# dbus detection
+if test "$dbus" != "no" ; then
+  cat > $TMPC <<EOF
+#include <glib.h>
+#include <glib-object.h>
+#include <dbus/dbus-glib.h>
+#include <dbus/dbus-glib-lowlevel.h>
+int main(void) { DBusGConnection *bus = dbus_g_bus_get (DBUS_BUS_SYSTEM, NULL);
+dbus_g_connection_unref(bus); return 0; }
+EOF
+  dbus_cflags=`$pkg_config --cflags gobject-2.0 dbus-glib-1 2> /dev/null`
+  dbus_libs=`$pkg_config --libs gobject-2.0 dbus-glib-1 2> /dev/null`
+  if compile_prog "$glib_cflags $dbus_cflags" "$glib_libs $dbus_libs" ; then
+    dbus=yes
+    QEMU_CFLAGS="$QEMU_CFLAGS $dbus_cflags"
+    libs_softmmu="$dbus_libs $libs_softmmu"
+    libs_tools="$dbus_libs $libs_tools"
+  else
+    if test "$dbus" = "yes" ; then
+      feature_not_found "dbus"
+    fi
+    dbus=no
+  fi
+fi
+
+##########################################
 # libcap probe
 
 if test "$cap" != "no" ; then
@@ -2874,6 +2908,7 @@ echo "Audio drivers     $audio_drv_list"
 echo "Extra audio cards $audio_card_list"
 echo "Block whitelist   $block_drv_whitelist"
 echo "Mixer emulation   $mixemu"
+echo "DBus support      $dbus"
 echo "VNC support       $vnc"
 if test "$vnc" = "yes" ; then
     echo "VNC TLS support   $vnc_tls"
@@ -3219,6 +3254,10 @@ if test "$opengl" = "yes" ; then
   echo "CONFIG_OPENGL=y" >> $config_host_mak
 fi
 
+if test "$dbus" = "yes" ; then
+  echo "CONFIG_DBUS=y" >> $config_host_mak
+fi
+
 if test "$libiscsi" = "yes" ; then
   echo "CONFIG_LIBISCSI=y" >> $config_host_mak
 fi
-- 
1.7.7.6

[Qemu-devel] [PATCH 5/5] raw-posix: unmount CD-ROM filesystem via udisks

[Paolo Bonzini]

We need to use O_EXCL in order to suppress event generation in the
kernel.  However, O_EXCL by definition fails when the CD-ROM drive
is mounted.  Automatically unmount it when it is passed through to
the guest.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 Makefile.objs            |    3 +
 block.c                  |    7 +++
 block/raw-posix-udisks.c |  105 ++++++++++++++++++++++++++++++++++++++++++++++
 block/raw-posix-udisks.h |   39 +++++++++++++++++
 block/raw-posix.c        |   17 ++++++-
 5 files changed, 168 insertions(+), 3 deletions(-)
 create mode 100644 block/raw-posix-udisks.c
 create mode 100644 block/raw-posix-udisks.h

diff --git a/Makefile.objs b/Makefile.objs
index ec35320..a30a80d 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -41,6 +41,9 @@ block-nested-$(CONFIG_POSIX) += raw-posix.o
 block-nested-$(CONFIG_LIBISCSI) += iscsi.o
 block-nested-$(CONFIG_CURL) += curl.o
 block-nested-$(CONFIG_RBD) += rbd.o
+ifeq ($(CONFIG_DBUS),y)
+block-nested-$(CONFIG_LINUX) += raw-posix-udisks.o
+endif
 
 block-obj-y +=  $(addprefix block/, $(block-nested-y))
 
diff --git a/block.c b/block.c
index 3621d11..6dad6b5 100644
--- a/block.c
+++ b/block.c
@@ -42,6 +42,10 @@
 #endif
 #endif
 
+#ifdef CONFIG_DBUS
+#include <glib-object.h>
+#endif
+
 #ifdef _WIN32
 #include <windows.h>
 #endif
@@ -3278,6 +3282,9 @@ BlockDriverAIOCB *bdrv_aio_discard(BlockDriverState *bs,
 void bdrv_init(void)
 {
     module_call_init(MODULE_INIT_BLOCK);
+#ifdef CONFIG_DBUS
+    g_type_init();
+#endif
 }
 
 void bdrv_init_with_whitelist(void)
diff --git a/block/raw-posix-udisks.c b/block/raw-posix-udisks.c
new file mode 100644
index 0000000..3ab3cbe
--- /dev/null
+++ b/block/raw-posix-udisks.c
@@ -0,0 +1,105 @@
+/*
+ * Udisks interaction for CD-ROM unmounting
+ *
+ * Copyright (c) 2012 Red Hat, Inc.
+ *
+ * Author: Paolo Bonzini  <pbonzini@redhat.com>
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include <glib.h>
+#include <glib-object.h>
+#include <stdlib.h>
+#include <stdio.h>
+#include <errno.h>
+
+#include <dbus/dbus-glib.h>
+#include <dbus/dbus-glib-lowlevel.h>
+#include "block/raw-posix-udisks.h"
+
+static void udisks_find_device_by_device_file(DBusGConnection *bus,
+                                              const char *path,
+                                              char **object_path,
+                                              GError **error)
+{
+    DBusGProxy *proxy;
+    proxy = dbus_g_proxy_new_for_name (bus, "org.freedesktop.UDisks",
+                                       "/org/freedesktop/UDisks",
+                                       "org.freedesktop.UDisks");
+    dbus_g_proxy_call (proxy, "FindDeviceByDeviceFile", error,
+                       G_TYPE_STRING, path, G_TYPE_INVALID,
+                       DBUS_TYPE_G_OBJECT_PATH, object_path, G_TYPE_INVALID);
+    g_object_unref (proxy);
+}
+
+static void udisks_device_filesystem_unmount(DBusGConnection *bus,
+                                             const char *object_path,
+                                             GError **error)
+{
+    DBusGProxy *proxy;
+    proxy = dbus_g_proxy_new_for_name (bus, "org.freedesktop.UDisks",
+                                       object_path,
+                                       "org.freedesktop.UDisks.Device");
+    dbus_g_proxy_call (proxy, "FilesystemUnmount", error,
+                       G_TYPE_STRV, NULL, G_TYPE_INVALID,
+                       G_TYPE_INVALID);
+    g_object_unref (proxy);
+}
+
+int
+udisks_unmount (const char *path)
+{
+    DBusGConnection *bus;
+    char *object_path;
+    GError *error;
+    int ret;
+
+    error = NULL;
+    bus = dbus_g_bus_get (DBUS_BUS_SYSTEM, &error);
+    if (bus == NULL) {
+        g_warning ("Couldn't connect to system bus: %s", error->message);
+        ret = -EACCES;
+        goto out;
+    }
+
+    udisks_find_device_by_device_file(bus, path, &object_path, &error);
+    if (error || !object_path) {
+        ret = -ENODEV;
+        goto out;
+    }
+
+    udisks_device_filesystem_unmount(bus, object_path, &error);
+    if (error) {
+        g_print ("Unmount failed: %s\n", error->message);
+        ret = -EBUSY;
+        goto out;
+      }
+
+    ret = 0; /* success */
+out:
+    g_free (object_path);
+    if (error != NULL) {
+        g_error_free (error);
+    }
+    if (bus != NULL) {
+        dbus_g_connection_unref (bus);
+    }
+    return ret;
+}
diff --git a/block/raw-posix-udisks.h b/block/raw-posix-udisks.h
new file mode 100644
index 0000000..47dd638
--- /dev/null
+++ b/block/raw-posix-udisks.h
@@ -0,0 +1,39 @@
+/*
+ * Udisks interaction for CD-ROM unmounting
+ *
+ * Copyright (c) 2012 Red Hat, Inc.
+ *
+ * Author: Paolo Bonzini  <pbonzini@redhat.com>
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#ifndef QEMU_UDISKS_H
+#define QEMU_UDISKS_H 1
+
+#include "config-host.h"
+#include <errno.h>
+
+#ifdef CONFIG_DBUS
+int udisks_unmount (const char *path);
+#else
+static int udisks_unmount (const char *path) { return -ENOSYS; }
+#endif
+
+#endif
diff --git a/block/raw-posix.c b/block/raw-posix.c
index 1b51bd4..c61d07a 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -28,6 +28,7 @@
 #include "block_int.h"
 #include "module.h"
 #include "block/raw-posix-aio.h"
+#include "block/raw-posix-udisks.h"
 
 #ifdef CONFIG_COCOA
 #include <paths.h>
@@ -1069,15 +1070,25 @@ static int cdrom_open(BlockDriverState *bs, const char *filename, int flags)
 {
     BDRVRawState *s = bs->opaque;
     int rc;
+    int i;
 
     s->type = FTYPE_CD;
 
     /* open will not fail even if no CD is inserted, so add O_NONBLOCK.  First
      * 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;
+    for (i = 0; i < 20; i++) {
+        rc = raw_open_common(bs, filename, flags, O_NONBLOCK | O_EXCL);
+        if (rc == 0) {
+            break;
+        }
+        if (rc != -EBUSY) {
+            return rc;
+        }
+        if (i == 0 && udisks_unmount(filename) < 0) {
+            break;
+        }
+        usleep(100 * 1000);
     }
 
     s->cd.manage_door = false;
-- 
1.7.7.6

Re: [Qemu-devel] [PATCH 5/5] raw-posix: unmount CD-ROM filesystem via udisks

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> We need to use O_EXCL in order to suppress event generation in the
> kernel.  However, O_EXCL by definition fails when the CD-ROM drive
> is mounted.  Automatically unmount it when it is passed through to
> the guest.

I'm not sure automatic unmount is worth the trouble, and I'm even less
sure making the auto-unmount mandatory is a smart move.

Re: [Qemu-devel] [PATCH 5/5] raw-posix: unmount CD-ROM filesystem via udisks

[Paolo Bonzini]

On 02/10/2012 01:51 PM, Markus Armbruster wrote:
> > We need to use O_EXCL in order to suppress event generation in the
> > kernel.  However, O_EXCL by definition fails when the CD-ROM drive
> > is mounted.  Automatically unmount it when it is passed through to
> > the guest.
>
> I'm not sure automatic unmount is worth the trouble, and I'm even less
> sure making the auto-unmount mandatory is a smart move.

Getting O_EXCL access is worth the trouble.  Everything else is just 
means to an end. :)

Paolo