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

[Pino Toscano]

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