Libguestfs - Jul 2009 - [PATCH] Fix broken qemu <= 0.10 which randomly adds a CD-ROM device to the appliance

qemu <= 0.10 randomly adds a CD-ROM device to the appliance because of
this bit of code:

http://git.savannah.gnu.org/cgit/qemu.git/tree/vl.c?h=stable-0.10#n5495

Thankfully this code has been removed from upstream qemu.

Anyway I'm not quite sure why we never saw this before - it seems like
this code didn't exist in the versions of qemu that were in Fedora, or
somehow this code only runs some of the time.  But anyway, this breaks
the tests in Fedora 11 and RHEL 5.3 right now.

The attached code attempts to work around this stupidity by adding
checks in list_devices and list_partitions to see if the device is
openable, and ignoring any which aren't.  These fake, empty CD drives
fail this extra check with error "No medium found", so they get
ignored.

[I'm still running all the tests with this patch, so not quite sure if
it's correct.]

Rich.

-- 
Richard Jones, Emerging Technologies, Red Hat  http://et.redhat.com/~rjones
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into Xen guests.
http://et.redhat.com/~rjones/virt-p2v
-------------- next part --------------
>From 6bb26f2d253b77570c2b9291021db4364597b6df Mon Sep 17 00:00:00 2001
From: Richard Jones <rjones at centos5x32.home.annexia.org>
Date: Wed, 29 Jul 2009 15:02:16 +0100
Subject: [PATCH] Don't show empty CD devices (RHBZ#514505).

---
 daemon/devsparts.c |   27 +++++++++++++++++++++++++++
 1 files changed, 27 insertions(+), 0 deletions(-)

diff --git a/daemon/devsparts.c b/daemon/devsparts.c
index 32d2fa8..a198ccd 100644
--- a/daemon/devsparts.c
+++ b/daemon/devsparts.c
@@ -37,6 +37,7 @@ do_list_devices (void)
   DIR *dir;
   struct dirent *d;
   char buf[256];
+  int fd;
 
   dir = opendir ("/sys/block");
   if (!dir) {
@@ -49,6 +50,18 @@ do_list_devices (void)
 	strncmp (d->d_name, "hd", 2) == 0 ||
 	strncmp (d->d_name, "vd", 2) == 0) {
       snprintf (buf, sizeof buf, "/dev/%s", d->d_name);
+
+      /* RHBZ#514505: Some versions of qemu <= 0.10 device to add a
+       * CD-ROM device even though we didn't request it.  Try to
+       * detect this by seeing if the device contains media.
+       */
+      fd = open (buf, O_RDONLY);
+      if (fd == -1) {
+	perror (buf);
+	continue;
+      }
+      close (fd);
+
       if (add_string (&r, &size, &alloc, buf) == -1) {
 	closedir (dir);
 	return NULL;
@@ -79,6 +92,7 @@ do_list_partitions (void)
   DIR *dir, *dir2;
   struct dirent *d;
   char buf[256], devname[256];
+  int fd;
 
   dir = opendir ("/sys/block");
   if (!dir) {
@@ -90,6 +104,19 @@ do_list_partitions (void)
     if (strncmp (d->d_name, "sd", 2) == 0 ||
 	strncmp (d->d_name, "hd", 2) == 0 ||
 	strncmp (d->d_name, "vd", 2) == 0) {
+      snprintf (buf, sizeof buf, "/dev/%s", d->d_name);
+
+      /* RHBZ#514505: Some versions of qemu <= 0.10 device to add a
+       * CD-ROM device even though we didn't request it.  Try to
+       * detect this by seeing if the device contains media.
+       */
+      fd = open (buf, O_RDONLY);
+      if (fd == -1) {
+	perror (buf);
+	continue;
+      }
+      close (fd);
+
       strncpy (devname, d->d_name, sizeof devname);
       devname[sizeof devname - 1] = '\0';
 
-- 
1.5.5.6

Richard W.M. Jones wrote:
>>From 6bb26f2d253b77570c2b9291021db4364597b6df Mon Sep 17 00:00:00 2001
> From: Richard Jones <rjones at centos5x32.home.annexia.org>
> Date: Wed, 29 Jul 2009 15:02:16 +0100
> Subject: [PATCH] Don't show empty CD devices (RHBZ#514505).
>
> ---
>  daemon/devsparts.c |   27 +++++++++++++++++++++++++++
>  1 files changed, 27 insertions(+), 0 deletions(-)
>
> diff --git a/daemon/devsparts.c b/daemon/devsparts.c
> index 32d2fa8..a198ccd 100644
> --- a/daemon/devsparts.c
> +++ b/daemon/devsparts.c
> @@ -37,6 +37,7 @@ do_list_devices (void)
>    DIR *dir;
>    struct dirent *d;
>    char buf[256];
> +  int fd;
>
>    dir = opendir ("/sys/block");
>    if (!dir) {
> @@ -49,6 +50,18 @@ do_list_devices (void)
>  	strncmp (d->d_name, "hd", 2) == 0 ||
>  	strncmp (d->d_name, "vd", 2) == 0) {
>        snprintf (buf, sizeof buf, "/dev/%s", d->d_name);
> +
> +      /* RHBZ#514505: Some versions of qemu <= 0.10 device to add a
> +       * CD-ROM device even though we didn't request it.  Try to
> +       * detect this by seeing if the device contains media.
> +       */
> +      fd = open (buf, O_RDONLY);
This all looks fine.  (modulo s/device to // in the comment)
However, imho it would be slightly better
(easier to read/review) if "fd" were declared at least
in the inner scope where it is used.
Personally, I'd go all the way and do this:

         int fd = open (buf, O_RDONLY);

assuming you're ok with c99 decl-after-stmt.

Hoping I'm not being too presumptuous, here's a patch:
>From aed1ab4721ab4b60081f539bd0023f64616ccd7e Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering at redhat.com>
Date: Wed, 29 Jul 2009 14:48:45 -0400
Subject: [PATCH] fix comments; move declarations

* daemon/devsparts.c (do_list_devices, do_list_partitions):
Remove stray words in comments.
Move declarations down to definition.
---
 daemon/devsparts.c |   10 ++++------
 1 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/daemon/devsparts.c b/daemon/devsparts.c
index a198ccd..33579ba 100644
--- a/daemon/devsparts.c
+++ b/daemon/devsparts.c
@@ -37,7 +37,6 @@ do_list_devices (void)
   DIR *dir;
   struct dirent *d;
   char buf[256];
-  int fd;

   dir = opendir ("/sys/block");
   if (!dir) {
@@ -51,11 +50,11 @@ do_list_devices (void)
 	strncmp (d->d_name, "vd", 2) == 0) {
       snprintf (buf, sizeof buf, "/dev/%s", d->d_name);

-      /* RHBZ#514505: Some versions of qemu <= 0.10 device to add a
+      /* RHBZ#514505: Some versions of qemu <= 0.10 add a
        * CD-ROM device even though we didn't request it.  Try to
        * detect this by seeing if the device contains media.
        */
-      fd = open (buf, O_RDONLY);
+      int fd = open (buf, O_RDONLY);
       if (fd == -1) {
 	perror (buf);
 	continue;
@@ -92,7 +91,6 @@ do_list_partitions (void)
   DIR *dir, *dir2;
   struct dirent *d;
   char buf[256], devname[256];
-  int fd;

   dir = opendir ("/sys/block");
   if (!dir) {
@@ -106,11 +104,11 @@ do_list_partitions (void)
 	strncmp (d->d_name, "vd", 2) == 0) {
       snprintf (buf, sizeof buf, "/dev/%s", d->d_name);

-      /* RHBZ#514505: Some versions of qemu <= 0.10 device to add a
+      /* RHBZ#514505: Some versions of qemu <= 0.10 add a
        * CD-ROM device even though we didn't request it.  Try to
        * detect this by seeing if the device contains media.
        */
-      fd = open (buf, O_RDONLY);
+      int fd = open (buf, O_RDONLY);
       if (fd == -1) {
 	perror (buf);
 	continue;
--
1.6.3.3

On 29/07/09 15:08, Richard W.M. Jones wrote:
> qemu<= 0.10 randomly adds a CD-ROM device to the appliance because of
> this bit of code:
>
> http://git.savannah.gnu.org/cgit/qemu.git/tree/vl.c?h=stable-0.10#n5495
>
I'm not convinced this is the right fix. list_devices is supposed to 
list devices, and for good or ill, the CD-ROM device is present. What is 
the contract of list_devices supposed to be?

Matt
-- 
Matthew Booth, RHCA, RHCSS
Red Hat Engineering, Virtualisation Team

M:       +44 (0)7977 267231
GPG ID:  D33C3490
GPG FPR: 3733 612D 2D05 5458 8A8A 1600 3441 EA19 D33C 3490