9:08 a.m.
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
3:28 p.m.
Richard W.M. Jones wrote:
>From 6bb26f2d253b77570c2b9291021db4364597b6df Mon Sep 17 00:00:00
2001
From: Richard Jones <rjones(a)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(a)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
3:14 a.m.
New subject: [PATCH] Fix broken qemu <= 0.10 which randomly adds a CD-ROM device to the appliance
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
3:38 a.m.
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
4:54 a.m.
New subject: [PATCH] Fix broken qemu <= 0.10 which randomly adds a CD-ROM device to the appliance
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
5:20 a.m.
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
6:23 a.m.
New subject: [PATCH] Fix broken qemu <= 0.10 which randomly adds a CD-ROM device to the appliance
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
7:31 a.m.
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
8:02 a.m.
.. 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
8:17 a.m.
New subject: [PATCH] Fix broken qemu <= 0.10 which randomly adds a CD-ROM device to the appliance
On Thu, Jul 30, 2009 at 3:38 AM, Richard W.M. Jones <rjones(a)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
5408
days inactive
5409
days old
9 comments
4 participants
participants (4)
-
Charles Duffy -
Jim Meyering -
Matthew Booth -
Richard W.M. Jones