Libguestfs - Dec 2013 - [PATCH] drives: add CD-ROM disk images as read-only drives (RHBZ#563450).

On Saturday 14 December 2013 22:50:28 Richard W.M. Jones
wrote:
> On Fri, Dec 13, 2013 at 04:32:49PM +0100, Pino Toscano wrote:
> > The current add_cdrom way basically appends a new raw "-cdrom
/path"
> > parameter to the qemu invocation (even when using libvirt as
> > backend), hence such images are seen as "CD-ROM drives"
inside the
> > appliance. However, there is no need for such particular behaviour,
> > as they need to be handled as normal (read-only) drives.
> > 
> > Adding CD-ROM disk images as drives also changes the device names
> > used for them inside the appliance from /dev/srN to the usual e.g.
> > /dev/sdX.
> > 
> > These changes fix different issues:
> > - it is possible to start guestfish without adding disks with -a,
> > then> 
> >   just add-cdrom and run
> > 
> > - list-devices does not cause guestfishd to crash when sorting the
> > list
> s/guestfishd/guestfsd/
> 
> >   of devices (exposed by the test case in RHBZ#563450)
> > 
> > - the result of list-devices now reflects the order images were
> > added
> > 
> >   (RHBZ#563450)
> > 
> > Add two small regression tests for the fixes described above.
> > ---
> > 
> >  src/drives.c                     |  7 +-----
> >  tests/regressions/Makefile.am    |  2 ++
> >  tests/regressions/rhbz563450.sh  | 54
> >  ++++++++++++++++++++++++++++++++++++++++
> >  tests/regressions/rhbz563450b.sh | 43
> >  ++++++++++++++++++++++++++++++++ 4 files changed, 100
> >  insertions(+), 6 deletions(-)
> >  create mode 100755 tests/regressions/rhbz563450.sh
> >  create mode 100755 tests/regressions/rhbz563450b.sh
> > 
> > diff --git a/src/drives.c b/src/drives.c
> > index 16798a3..4b65dda 100644
> > --- a/src/drives.c
> > +++ b/src/drives.c
> > @@ -1087,12 +1087,7 @@ guestfs__add_cdrom (guestfs_h *g, const char
> > *filename)> 
> >      return -1;
> >    
> >    }
> > 
> > -  if (access (filename, F_OK) == -1) {
> > -    perrorf (g, "%s", filename);
> > -    return -1;
> > -  }
> > -
> > -  return guestfs_config (g, "-cdrom", filename);
> > +  return guestfs__add_drive_ro (g, filename);
> > 
> >  }
> 
> I don't think this implementation is quite right because it leaves the
> [now] bogus check for ':' in the filename.  guestfs_add_drive_ro
can
> handle ':' in the filename (because it creates an overlay):
I see, I was being conservative with the current behaviour, since not 
allowing colons is what add-cdrom enforces and thus nobody could have 
used them so far.
> I think it's better to remove the contents of the guestfs__add_cdrom
> function completely, so the function will become just:
> 
>   int
>   guestfs__add_cdrom (guestfs_h *g, const char *filename)
>   {
>     return guestfs__add_drive_ro (g, filename);
>   }
With the above, this makes sense now.
> Also, should we update the documentation?
> 
> http://libguestfs.org/guestfs.3.html#guestfs_add_cdrom
> 
> Maybe or maybe not worth it.
Rgiht, worth it, updated.
> > +# https://bugzilla.redhat.com/show_bug.cgi?id=563450
> > +# Test the order of added images
> > +
> > +set -e
> > +export LANG=C
> > +
> > +rm -f test.out
> > +
> > +../../fish/guestfish --ro > test.out <<EOF
> > +add-drive-ro ../guests/fedora.img
> > +add-cdrom ../data/test.iso
> > +add-drive-ro ../guests/debian.img
> 
> Earlier in the test, you'll probably need to add this:
> 
>   if [ ! -s ../guests/fedora.img -o ! -s ../guests/debian.img ]; then
>       echo "$0: test skipped because there is no fedora.img or
> debian.img" exit 77
>   fi
It seems there are few more tests that don't check for the existence of 
test guests and isos:
  align/test-virt-alignment-scan.sh
  cat/test-virt-cat.sh
  cat/test-virt-filesystems.sh
  cat/test-virt-ls.sh
  df/test-virt-df.sh
  edit/test-virt-edit.sh
  fish/test-find0.sh
  fish/test-inspect.sh
  fish/test-read-file.sh
  fish/test-run.sh
  fish/test-upload-to-dir.sh
  fuse/test-fuse-umount-race.sh
  inspector/test-virt-inspector.sh
  sysprep/test-virt-sysprep-passwords.sh
  sysprep/test-virt-sysprep-script.sh
  sysprep/test-virt-sysprep.sh
  tests/md/test-inspect-fstab-md.sh
  tests/md/test-inspect-fstab.sh
  tests/mountable/test-mountable-inspect.sh
  tests/regressions/rhbz789960.sh
  tools/test-virt-list-filesystems.sh
  tools/test-virt-tar.sh
(and also df/test-virt-df-guests.sh which uses the generated guest.xml)

Should I add checks in all of them, or nor add them in the new ones?
In case of the former, I'd add them in the new tests together with the 
others.

-- 
Pino Toscano

The current add_cdrom way basically appends a new raw "-cdrom /path"
parameter to the qemu invocation (even when using libvirt as backend),
hence such images are seen as "CD-ROM drives" inside the appliance.
However, there is no need for such particular behaviour, as they need to
be handled as normal (read-only) drives.

Adding CD-ROM disk images as drives also changes the device names used
for them inside the appliance from /dev/srN to the usual e.g. /dev/sdX.

These changes fix different issues:
- it is possible to start guestfish without adding disks with -a, then
  just add-cdrom and run
- list-devices does not cause guestfsd to crash when sorting the list
  of devices (exposed by the test case in RHBZ#563450)
- the result of list-devices now reflects the order images were added
  (RHBZ#563450)

add_cdrom is still deprecated, but now in favour of add_drive_ro
(instead of add_drive), with its documentation reflecting that.

Add two small regression tests for the fixes described above.
---
 generator/actions.ml             |  6 ++---
 src/drives.c                     | 13 +---------
 tests/regressions/Makefile.am    |  2 ++
 tests/regressions/rhbz563450.sh  | 54 ++++++++++++++++++++++++++++++++++++++++
 tests/regressions/rhbz563450b.sh | 43 ++++++++++++++++++++++++++++++++
 5 files changed, 103 insertions(+), 15 deletions(-)
 create mode 100755 tests/regressions/rhbz563450.sh
 create mode 100755 tests/regressions/rhbz563450b.sh

diff --git a/generator/actions.ml b/generator/actions.ml
index cb6d137..b884efb 100644
--- a/generator/actions.ml
+++ b/generator/actions.ml
@@ -241,14 +241,14 @@ Do not call this.  See: C<guestfs_shutdown>
instead." };
   { defaults with
     name = "add_cdrom";
     style = RErr, [String "filename"], [];
-    deprecated_by = Some "add_drive"; config_only = true;
+    deprecated_by = Some "add_drive_ro"; config_only = true;
     blocking = false;
     shortdesc = "add a CD-ROM disk image to examine";
     longdesc = "\
 This function adds a virtual CD-ROM disk image to the guest.
 
-B<Do not use this function!>  ISO files are just ordinary
-read-only disk images.  Use C<guestfs_add_drive_ro> instead." };
+The image is added as read-only drive, so this function is equivalent
+of C<guestfs_add_drive_ro>." };
 
   { defaults with
     name = "add_drive_ro";
diff --git a/src/drives.c b/src/drives.c
index 16798a3..cddde4f 100644
--- a/src/drives.c
+++ b/src/drives.c
@@ -1081,18 +1081,7 @@ guestfs__add_drive_scratch (guestfs_h *g, int64_t size,
 int
 guestfs__add_cdrom (guestfs_h *g, const char *filename)
 {
-  if (strchr (filename, ':') != NULL) {
-    error (g, _("filename cannot contain ':' (colon) character.
"
-                "This is a limitation of qemu."));
-    return -1;
-  }
-
-  if (access (filename, F_OK) == -1) {
-    perrorf (g, "%s", filename);
-    return -1;
-  }
-
-  return guestfs_config (g, "-cdrom", filename);
+  return guestfs__add_drive_ro (g, filename);
 }
 
 /* Depending on whether we are hotplugging or not, this function
diff --git a/tests/regressions/Makefile.am b/tests/regressions/Makefile.am
index 5b0d1c3..741f45f 100644
--- a/tests/regressions/Makefile.am
+++ b/tests/regressions/Makefile.am
@@ -21,6 +21,8 @@ TESTS = \
 	rhbz501893 \
 	rhbz503169c13.sh \
 	rhbz557655.sh \
+	rhbz563450.sh \
+	rhbz563450b.sh \
 	rhbz576879.sh \
 	rhbz578407.sh \
 	rhbz580246.sh \
diff --git a/tests/regressions/rhbz563450.sh b/tests/regressions/rhbz563450.sh
new file mode 100755
index 0000000..6fa6f2b
--- /dev/null
+++ b/tests/regressions/rhbz563450.sh
@@ -0,0 +1,54 @@
+#!/bin/bash -
+# libguestfs
+# Copyright (C) 2013 Red Hat Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+
+# https://bugzilla.redhat.com/show_bug.cgi?id=563450
+# Test the order of added images
+
+set -e
+export LANG=C
+
+rm -f test.out
+
+../../fish/guestfish --ro > test.out <<EOF
+add-drive-ro ../guests/fedora.img
+add-cdrom ../data/test.iso
+add-drive-ro ../guests/debian.img
+
+run
+
+list-devices
+echo ----
+list-partitions
+
+ping-daemon
+EOF
+
+if [ "$(cat test.out)" != "/dev/sda
+/dev/sdb
+/dev/sdc
+----
+/dev/sda1
+/dev/sda2
+/dev/sdc1
+/dev/sdc2" ]; then
+    echo "$0: unexpected output:"
+    cat test.out
+    exit 1
+fi
+
+rm -f test.out
diff --git a/tests/regressions/rhbz563450b.sh b/tests/regressions/rhbz563450b.sh
new file mode 100755
index 0000000..bae641a
--- /dev/null
+++ b/tests/regressions/rhbz563450b.sh
@@ -0,0 +1,43 @@
+#!/bin/bash -
+# libguestfs
+# Copyright (C) 2013 Red Hat Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+
+# https://bugzilla.redhat.com/show_bug.cgi?id=563450
+# Test only CD-ROM disk images can be added
+
+set -e
+export LANG=C
+
+rm -f test.out
+
+../../fish/guestfish --ro > test.out <<EOF
+add-cdrom ../data/test.iso
+
+run
+
+list-devices
+
+ping-daemon
+EOF
+
+if [ "$(cat test.out)" != "/dev/sda" ]; then
+    echo "$0: unexpected output:"
+    cat test.out
+    exit 1
+fi
+
+rm -f test.out
-- 
1.8.3.1

On Mon, Dec 16, 2013 at 01:42:51PM +0100, Pino Toscano
wrote:
> On Saturday 14 December 2013 22:50:28 Richard W.M. Jones wrote:
> > Earlier in the test, you'll probably need to add this:
> > 
> >   if [ ! -s ../guests/fedora.img -o ! -s ../guests/debian.img ]; then
> >       echo "$0: test skipped because there is no fedora.img or
> > debian.img" exit 77
> >   fi
> 
> It seems there are few more tests that don't check for the existence of
> test guests and isos:
>   align/test-virt-alignment-scan.sh
[...]

The tests/guests stuff is a bit of a mess:

(1) The windows.img can be created zero-sized if there's no NTFS
support.  The comment in tests/guests/guest-aux/make-windows-img.sh is
quite blasé saying "Nothing actually uses windows.img in the standard
build" which is not true at all!

(2) Lack of MD support would prevent us from creating the fedora-md*
guest.  I can't remember what exactly happens in this case.

(3) Lack of btrfs support would prevent us from creating the
fedora-btrfs.img guest.  It's quite common that btrfs is broken or
missing altogether, especially on less common distros.

(4) Lack of LVM support [yes, this can happen] can prevent us from
creating any Linux guest.

(5) Various files are derived from the test images, eg:

  builder/test-index
  builder/*.xz
  builder/*.qcow2
  builder/*.qcow2.xz

but these sometimes don't omit guests which we couldn't create.

(Note that tests/guests/guests-all-good.xml gets this right)

The builder/* ones should be created in the tests/guests directory,
and the virt-builder tests should use them out of that directory.

(6) UML doesn't support qcow2.

And of course ...

(7) Phony guests are nice for simple tests, but don't really test
inspection.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW

On Mon, Dec 16, 2013 at 01:43:54PM +0100, Pino Toscano
wrote:
> The current add_cdrom way basically appends a new raw "-cdrom
/path"
> parameter to the qemu invocation (even when using libvirt as backend),
> hence such images are seen as "CD-ROM drives" inside the
appliance.
> However, there is no need for such particular behaviour, as they need to
> be handled as normal (read-only) drives.
> 
> Adding CD-ROM disk images as drives also changes the device names used
> for them inside the appliance from /dev/srN to the usual e.g. /dev/sdX.
> 
> These changes fix different issues:
> - it is possible to start guestfish without adding disks with -a, then
>   just add-cdrom and run
> - list-devices does not cause guestfsd to crash when sorting the list
>   of devices (exposed by the test case in RHBZ#563450)
> - the result of list-devices now reflects the order images were added
>   (RHBZ#563450)
> 
> add_cdrom is still deprecated, but now in favour of add_drive_ro
> (instead of add_drive), with its documentation reflecting that.
> 
> Add two small regression tests for the fixes described above.
I still think those tests are going to need to check for
the existence and ≠ 0 size of ../guests/{debian,fedora}.img
(see previous email).

ACK with that change.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.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 KVM guests.
http://libguestfs.org/virt-v2v