[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
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: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
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 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
Added in 2392fae6803b9de4a4761509007c160339d57f50 and removed in be025369ad622867ecf08d5c9170e8e59d2d6294; now they should work with any backend, as the device names are normalised and not change the guestfish output depending on the used backend. Thanks Richard for the hints. --- tests/regressions/Makefile.am | 2 ++ tests/regressions/rhbz563450.sh | 59 ++++++++++++++++++++++++++++++++++++++++ tests/regressions/rhbz563450b.sh | 48 ++++++++++++++++++++++++++++++++ 3 files changed, 109 insertions(+) create mode 100755 tests/regressions/rhbz563450.sh create mode 100755 tests/regressions/rhbz563450b.sh 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..96fca8c --- /dev/null +++ b/tests/regressions/rhbz563450.sh @@ -0,0 +1,59 @@ +#!/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 + +if [ ! -s ../guests/fedora.img -o ! -s ../data/test.iso -o ! -s ../guests/debian.img ]; then + echo "$0: test skipped because there is no fedora.img nor test.iso nor debian.img" + exit 77 +fi + +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 | sed -r 's,^/dev/[abce-ln-z]+d,/dev/sd,' +echo ---- +list-partitions | sed -r 's,^/dev/[abce-ln-z]+d,/dev/sd,' + +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..7b890b1 --- /dev/null +++ b/tests/regressions/rhbz563450b.sh @@ -0,0 +1,48 @@ +#!/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 + +if [ ! -s ../data/test.iso ]; then + echo "$0: test skipped because there is no test.iso" + exit 77 +fi + +rm -f test.out + +../../fish/guestfish --ro > test.out <<EOF +add-cdrom ../data/test.iso + +run + +list-devices | sed -r 's,^/dev/[abce-ln-z]+d,/dev/sd,' + +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 Thu, Dec 19, 2013 at 04:30:57PM +0100, Pino Toscano wrote:
Added in 2392fae6803b9de4a4761509007c160339d57f50 and removed in be025369ad622867ecf08d5c9170e8e59d2d6294; now they should work with any backend, as the device names are normalised and not change the guestfish output depending on the used backend.
Thanks Richard for the hints.
ACK.
+../../fish/guestfish --ro > test.out <<EOF
It could also be done as: ../../fish/guestfish --ro | sed ... > test.out which would be one sed invocation. However I think that would hide any errors / segfaults in guestfish, so the way you've got it now seems more robust. 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#)
participants (2)
-
Pino Toscano -
Richard W.M. Jones