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 _______________________________________________ Libguestfs mailing list Libguestfs@redhat.com https://www.redhat.com/mailman/listinfo/libguestfs