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

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
  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);
 }
 
 /* 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 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):

  $ truncate -s 100M foo:bar
  $ guestfish --ro -a foo:bar 
  
  Welcome to guestfish, the guest filesystem shell for
  editing virtual machine filesystems and disk images.
  
  Type: 'help' for help on commands
        'man' to read the manual
        'quit' to quit the shell
  
  ><fs> run

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);
  }


[...]
> +# 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

---

Also, should we update the documentation?

http://libguestfs.org/guestfs.3.html#guestfs_add_cdrom

Maybe or maybe not worth it.

---

Patch looks good apart from these three small issues.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming blog: http://rwmj.wordpress.com
Fedora now supports 80 OCaml packages (the OPEN alternative to F#)

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

On Fri, Dec 13, 2013 at 04:32:49PM +0100, Pino Toscano
wrote:
> 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
There's a bug in both of these tests, so I had to remove them in order
to get a release out today.

'list-devices' doesn't canonicalize disk names (perhaps it should,
but
it doesn't).  Therefore if the appliance is using old virtio-blk it
will return disk names such as /dev/vda, and if the appliance is
running under UML it will return /dev/ubda (which was what failed in
'make check-release').

If you look at other tests such as:

 - df/test-virt-df.sh
 - tests/luks/test-luks-list.sh

they get around this by canonicalizing the device names (in different
ways) before comparing them.

If you correct the bug then we can put the tests back.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming blog: http://rwmj.wordpress.com
Fedora now supports 80 OCaml packages (the OPEN alternative to F#)

On Wednesday 18 December 2013 20:44:33 Richard W.M. Jones
wrote:
> On Fri, Dec 13, 2013 at 04:32:49PM +0100, Pino Toscano wrote:
> > 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
> 
> There's a bug in both of these tests, so I had to remove them in order
> to get a release out today.
> 
> 'list-devices' doesn't canonicalize disk names (perhaps it
should, but
> it doesn't).  Therefore if the appliance is using old virtio-blk it
> will return disk names such as /dev/vda, and if the appliance is
> running under UML it will return /dev/ubda (which was what failed in
> 'make check-release').
You are right, it slipped in my checks.
> If you look at other tests such as:
> 
>  - df/test-virt-df.sh
>  - tests/luks/test-luks-list.sh
> 
> they get around this by canonicalizing the device names (in different
> ways) before comparing them.
I've taken this approach, although IMHO replacing the whole output just 
once at the end would be a cleaner approach than spreading sed+regexp 
all over the guestfish invocations.
> If you correct the bug then we can put the tests back.
Sending updated patch.

-- 
Pino Toscano