[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
Richard W.M. Jones wrote:
From 6bb26f2d253b77570c2b9291021db4364597b6df Mon Sep 17 00:00:00 2001 From: Richard Jones <rjones@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@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
On Thu, Jul 30, 2009 at 09:14:07AM +0100, Matthew Booth wrote:
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.
If the caller called add_cdrom then the CD-ROM device would be both present _and_ contain media, so it would show up in the list. There's no way that I know of to add a CD-ROM file without media using an explicit qemu command line parameter. AFAIK such devices can only be added by this qemu stupidity.
What is the contract of list_devices supposed to be?
My expectation has been that it would list the devices which were added using add_drive* / add_cdrom calls. Certainly not that it would list those devices, plus some other random devices that qemu pulled out of thin air. We (sorta) enforced this contract by adding an explicit test that this was the case, and this test started to break when we had a version of qemu which was adding its own random CD-ROM device. Rich. -- Richard Jones, Emerging Technologies, Red Hat http://et.redhat.com/~rjones New in Fedora 11: Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 70 libraries supprt'd http://fedoraproject.org/wiki/MinGW http://www.annexia.org/fedora_mingw
On 30/07/09 09:38, Richard W.M. Jones wrote:
What is the contract of list_devices supposed to be?
My expectation has been that it would list the devices which were added using add_drive* / add_cdrom calls. Certainly not that it would list those devices, plus some other random devices that qemu pulled out of thin air.
We (sorta) enforced this contract by adding an explicit test that this was the case, and this test started to break when we had a version of qemu which was adding its own random CD-ROM device.
We already know without interrogation which devices we intended to be added. Presumably, then, list_devices is giving us some additional, guest specific information. Would it not be more robust to iterate over the list of expected devices looking for them explicitly and individually in the guest? 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
On Thu, Jul 30, 2009 at 10:54:40AM +0100, Matthew Booth wrote:
On 30/07/09 09:38, Richard W.M. Jones wrote:
What is the contract of list_devices supposed to be?
My expectation has been that it would list the devices which were added using add_drive* / add_cdrom calls. Certainly not that it would list those devices, plus some other random devices that qemu pulled out of thin air.
We (sorta) enforced this contract by adding an explicit test that this was the case, and this test started to break when we had a version of qemu which was adding its own random CD-ROM device.
We already know without interrogation which devices we intended to be added. Presumably, then, list_devices is giving us some additional, guest specific information.
Would it not be more robust to iterate over the list of expected devices looking for them explicitly and individually in the guest?
Unfortunately we don't know this .. We expose the full qemu command line via http://libguestfs.org/guestfs.3.html#guestfs_config . Rich. -- Richard Jones, Emerging Technologies, Red Hat http://et.redhat.com/~rjones virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://et.redhat.com/~rjones/virt-top
On 30/07/09 11:20, Richard W.M. Jones wrote:
Unfortunately we don't know this .. We expose the full qemu command line via http://libguestfs.org/guestfs.3.html#guestfs_config .
Ok, tackling this from the other end: What's the list_devices use case that this breaks? 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
On Thu, Jul 30, 2009 at 12:23:06PM +0100, Matthew Booth wrote:
On 30/07/09 11:20, Richard W.M. Jones wrote:
Unfortunately we don't know this .. We expose the full qemu command line via http://libguestfs.org/guestfs.3.html#guestfs_config .
Ok, tackling this from the other end: What's the list_devices use case that this breaks?
The one where we add a squashfs as the "last" device, and then get that device's real name from inside the guest using $g->list_devices() using whatever the Perl syntax was for getting the last element in a list. A more robust way to do this is to use ISO images and volume IDs ... Anyhow I still can't see how it's useful either for qemu to be adding random unrequested devices, or to return devices with no media in them from guestfs_list_devices, and quite rightly the tests were getting confused when this happened. Rich. -- Richard Jones, Emerging Technologies, Red Hat http://et.redhat.com/~rjones libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://et.redhat.com/~rjones/libguestfs/ See what it can do: http://et.redhat.com/~rjones/libguestfs/recipes.html
.. but if you wanted to add a 'guestfs_list_devices_all' call, then that would make sense to me. Rich. -- Richard Jones, Emerging Technologies, Red Hat http://et.redhat.com/~rjones libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://et.redhat.com/~rjones/libguestfs/ See what it can do: http://et.redhat.com/~rjones/libguestfs/recipes.html
On Thu, Jul 30, 2009 at 3:38 AM, Richard W.M. Jones <rjones@redhat.com>wrote:
There's no way that I know of to add a CD-ROM file without media using an explicit qemu command line parameter. AFAIK such devices can only be added by this qemu stupidity.
-drive if=ide,index=1,media=cdrom ^^ no file= parameter given
participants (4)
-
Charles Duffy -
Jim Meyering -
Matthew Booth -
Richard W.M. Jones