[Qemu-devel] [PATCH 0/2] Guess host device type from guest device type for block devices

[Qemu-devel] [PATCH 0/2] Guess host device type from guest device type for block devices

[Eduardo Habkost]

This fixes the following issue:
https://bugzilla.redhat.com/show_bug.cgi?id=473154

Sometimes a CD-ROM drive path doesn't start with /dev/cdrom, causing problems
if a disk isn't inserted on the drive. With this patch, the floppy and cdrom
drivers are used if using a host block device as backend and the
virtual device type is floppy or CD-ROM.

Example:

 $ ls -l /dev/cdrom /dev/hda
 lrwxrwxrwx 1 root root    3 Jun 17 10:20 /dev/cdrom -> hda
 brw-rw---- 1 root disk 3, 0 Jun 17 10:20 /dev/hda

Before this series (CD-ROM drive with no disk):

 $ sudo qemu-system-x86_64 -cdrom /dev/cdrom
 [works]
 $ sudo qemu-system-x86_64 -cdrom /dev/hda
 qemu: could not open disk image /dev/hda

After this series (with no disk on the CD-ROM drive, too):

 $ sudo qemu-system-x86_64 -cdrom /dev/hda
 [works]

This is based on a fix from Cole Robinson, submitted here:
http://marc.info/?l=qemu-devel&m=124458617900905

There are other two points where I think the code could be improved:

- Redundant stat() calls: stat() is called at hdev_probe_device(),
  and called again on cdrom_probe_device() and floppy_probe_device()
- Duplicated cdrom driver code for Linux and FreeBSD: I don't like #ifdefs, but
  I don't like the amount of duplicated code, either.


Eduardo Habkost (2):
  Add a BlockDriverState parameter to bdrv_probe_device()
  Guess host device type from requested guest device type

 block.c           |    6 +++---
 block/raw-posix.c |   38 +++++++++++++++++++++++++++++---------
 block/raw-win32.c |    2 +-
 block_int.h       |    2 +-
 4 files changed, 34 insertions(+), 14 deletions(-)

-- 
Eduardo

[Qemu-devel] [PATCH 1/2] Add a BlockDriverState parameter to bdrv_probe_device()

[Eduardo Habkost]

Some probe functions may behave differently depending on the drive
configuration. For example, if a block device is used as backend for a virtual
cdrom device, the cdrom driver may be used.

Changes not tested on Windows or FreeBSD, but I don't expect them to cause any
problems.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 block.c           |    6 +++---
 block/raw-posix.c |    8 ++++----
 block/raw-win32.c |    2 +-
 block_int.h       |    2 +-
 4 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/block.c b/block.c
index aca5a6d..fa890a7 100644
--- a/block.c
+++ b/block.c
@@ -253,14 +253,14 @@ static BlockDriver *find_protocol(const char *filename)
  * Detect host devices. By convention, /dev/cdrom[N] is always
  * recognized as a host CDROM.
  */
-static BlockDriver *find_hdev_driver(const char *filename)
+static BlockDriver *find_hdev_driver(BlockDriverState *bs, const char *filename)
 {
     int score_max = 0, score;
     BlockDriver *drv = NULL, *d;
 
     for (d = first_drv; d; d = d->next) {
         if (d->bdrv_probe_device) {
-            score = d->bdrv_probe_device(filename);
+            score = d->bdrv_probe_device(bs, filename);
             if (score > score_max) {
                 score_max = score;
                 drv = d;
@@ -397,7 +397,7 @@ int bdrv_open2(BlockDriverState *bs, const char *filename, int flags,
     if (flags & BDRV_O_FILE) {
         drv = find_protocol(filename);
     } else if (!drv) {
-        drv = find_hdev_driver(filename);
+        drv = find_hdev_driver(bs, filename);
         if (!drv) {
             drv = find_image_format(filename);
         }
diff --git a/block/raw-posix.c b/block/raw-posix.c
index fa1a394..bbcb6bc 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -955,7 +955,7 @@ kern_return_t GetBSDPath( io_iterator_t mediaIterator, char *bsdPath, CFIndex ma
 
 #endif
 
-static int hdev_probe_device(const char *filename)
+static int hdev_probe_device(BlockDriverState *bs, const char *filename)
 {
     struct stat st;
 
@@ -1201,7 +1201,7 @@ static int floppy_open(BlockDriverState *bs, const char *filename, int flags)
     return 0;
 }
 
-static int floppy_probe_device(const char *filename)
+static int floppy_probe_device(BlockDriverState *bs, const char *filename)
 {
     if (strstart(filename, "/dev/fd", NULL))
         return 100;
@@ -1285,7 +1285,7 @@ static int cdrom_open(BlockDriverState *bs, const char *filename, int flags)
     return raw_open_common(bs, filename, flags, O_NONBLOCK);
 }
 
-static int cdrom_probe_device(const char *filename)
+static int cdrom_probe_device(BlockDriverState *bs, const char *filename)
 {
     if (strstart(filename, "/dev/cd", NULL))
         return 100;
@@ -1381,7 +1381,7 @@ static int cdrom_open(BlockDriverState *bs, const char *filename, int flags)
     return 0;
 }
 
-static int cdrom_probe_device(const char *filename)
+static int cdrom_probe_device(BlockDriverState *bs, const char *filename)
 {
     if (strstart(filename, "/dev/cd", NULL) ||
             strstart(filename, "/dev/acd", NULL))
diff --git a/block/raw-win32.c b/block/raw-win32.c
index 72acad5..7c7e846 100644
--- a/block/raw-win32.c
+++ b/block/raw-win32.c
@@ -306,7 +306,7 @@ static int find_device_type(BlockDriverState *bs, const char *filename)
     }
 }
 
-static int hdev_probe_device(const char *filename)
+static int hdev_probe_device(BlockDriverState *bs, const char *filename)
 {
     if (strstart(filename, "/dev/cdrom", NULL))
         return 100;
diff --git a/block_int.h b/block_int.h
index 830b7e9..b70186d 100644
--- a/block_int.h
+++ b/block_int.h
@@ -48,7 +48,7 @@ struct BlockDriver {
     const char *format_name;
     int instance_size;
     int (*bdrv_probe)(const uint8_t *buf, int buf_size, const char *filename);
-    int (*bdrv_probe_device)(const char *filename);
+    int (*bdrv_probe_device)(BlockDriverState *bs, const char *filename);
     int (*bdrv_open)(BlockDriverState *bs, const char *filename, int flags);
     int (*bdrv_read)(BlockDriverState *bs, int64_t sector_num,
                      uint8_t *buf, int nb_sectors);
-- 
1.6.3.rc4.29.g8146

[Qemu-devel] [PATCH 2/2] Guess host device type from requested guest device type

[Eduardo Habkost]

This fixes the following issue:
https://bugzilla.redhat.com/show_bug.cgi?id=473154

Sometimes a CD-ROM drive path doesn't start with /dev/cdrom, causing problems
if a disk isn't inserted on the drive. With this patch, the floppy and cdrom
drivers are used if using a host block device as backend and the
virtual device type is floppy or CD-ROM.

Example:

 $ ls -l /dev/cdrom /dev/hda
 lrwxrwxrwx 1 root root    3 Jun 17 10:20 /dev/cdrom -> hda
 brw-rw---- 1 root disk 3, 0 Jun 17 10:20 /dev/hda

Before this patch (CD-ROM drive with no disk):

 $ sudo qemu-system-x86_64 -cdrom /dev/cdrom
 [works]
 $ sudo qemu-system-x86_64 -cdrom /dev/hda
 qemu: could not open disk image /dev/hda

After this patch (with no disk on the CD-ROM drive, too):

 $ sudo qemu-system-x86_64 -cdrom /dev/hda
 [works]

This is based on a fix from Cole Robinson, submitted here:
http://marc.info/?l=qemu-devel&m=124458617900905

The FreeBSD side of the cdrom driver changes is untested.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 block/raw-posix.c |   30 +++++++++++++++++++++++++-----
 1 files changed, 25 insertions(+), 5 deletions(-)

diff --git a/block/raw-posix.c b/block/raw-posix.c
index bbcb6bc..795d465 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -959,13 +959,12 @@ static int hdev_probe_device(BlockDriverState *bs, 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;
+        /* allow a dedicated driver (e.g. cdrom, floppy) to match with a higher
+         * priority
+         */
+        return 90;
     }
 
     return 0;
@@ -1203,8 +1202,15 @@ static int floppy_open(BlockDriverState *bs, const char *filename, int flags)
 
 static int floppy_probe_device(BlockDriverState *bs, const char *filename)
 {
+    struct stat st;
+
     if (strstart(filename, "/dev/fd", NULL))
         return 100;
+
+    if (bs->type == BDRV_TYPE_FLOPPY &&
+        stat(filename, &st) >= 0 && S_ISBLK(st.st_mode))
+        return 100;
+
     return 0;
 }
 
@@ -1287,8 +1293,15 @@ static int cdrom_open(BlockDriverState *bs, const char *filename, int flags)
 
 static int cdrom_probe_device(BlockDriverState *bs, const char *filename)
 {
+    struct stat st;
+
     if (strstart(filename, "/dev/cd", NULL))
         return 100;
+
+    if (bs->type == BDRV_TYPE_CDROM &&
+        stat(filename, &st) >= 0 && S_ISBLK(st.st_mode))
+        return 100;
+
     return 0;
 }
 
@@ -1383,9 +1396,16 @@ static int cdrom_open(BlockDriverState *bs, const char *filename, int flags)
 
 static int cdrom_probe_device(BlockDriverState *bs, const char *filename)
 {
+    struct stat st;
+
     if (strstart(filename, "/dev/cd", NULL) ||
             strstart(filename, "/dev/acd", NULL))
         return 100;
+
+    if (bs->type == BDRV_TYPE_CDROM &&
+        stat(filename, &st) >= 0 && S_ISBLK(st.st_mode))
+        return 100;
+
     return 0;
 }
 
-- 
1.6.3.rc4.29.g8146

[Qemu-devel] Re: [PATCH 0/2] Guess host device type from guest device type for block devices

[Anthony Liguori]

Eduardo Habkost wrote:
> This fixes the following issue:
> https://bugzilla.redhat.com/show_bug.cgi?id=473154
>
> Sometimes a CD-ROM drive path doesn't start with /dev/cdrom, causing problems
> if a disk isn't inserted on the drive. With this patch, the floppy and cdrom
> drivers are used if using a host block device as backend and the
> virtual device type is floppy or CD-ROM.
>   

Filename based probing is bad, but I think your solution is flawed too.  
It breaks the case where someone does:

qemu -hda /dev/volumes/RH5-iso

Where /dev/volumes/RH5-iso is just a plain LVM volume.  I can think of 
legitimate reasons to do this.

I think a better approach would be to try and issue a cdrom-specific 
ioctl without side-effects, and then use that probe to decide whether 
it's a cdrom.  For instance, CDROM_DRIVE_STATUS ought to be pretty safe.

Long term, I think we want to take a more thorough approach to host 
devices.  First, we should use fstat instead of filenames to determine 
whether something is a host device.

We should then use the info in fstat to determine what type of device it 
is.  Then we can use host-specific mechanisms to determine what the host 
device type is.

For instance, on Linux, the right thing to do is:

lstat(path) -> test -e /sys/dev/block/<major>:<minor>/capability -> 
capability & GENHD_FL_CD

Regards,

Anthony Liguori