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

2013-12-18 Thread Richard W.M. Jones 
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 000..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#)

___
Libguestfs mailing list
Libguestfs@redhat.com
https://www.redhat.com/mailman/listinfo/libguestfs

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

2013-12-16 Thread 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: Cguestfs_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.
 
-BDo not use this function!  ISO files are just ordinary
-read-only disk images.  Use Cguestfs_add_drive_ro instead. };
+The image is added as read-only drive, so this function is equivalent
+of Cguestfs_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 000..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

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

2013-12-14 Thread Richard W.M. Jones 
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#)

___
Libguestfs mailing list
Libguestfs@redhat.com
https://www.redhat.com/mailman/listinfo/libguestfs