1:18 p.m.
On 26/07/09 18:50, Richard W.M. Jones wrote:
On Fri, Jul 24, 2009 at 10:34:21PM +0100, Matthew Booth wrote:
> diff --git a/perl/lib/Sys/Guestfs/GuestOS/RedHat.pm
b/perl/lib/Sys/Guestfs/GuestOS/RedHat.pm
> +=head1 SYNOPSIS
> +
> + use Sys::Guestfs::GuestOS;
> +
> + $guestos = Sys::Guestfs::GuestOS->get_instance($os, $distro, $version)
> +
> +=head1 DESCRIPTION
> +
> +Sys::Guestfs::GuestOS provides a mechanism for querying and manipulating a
> +specific guest operating system.
[...]
This documentation is just duplicated from the parent class.
Yeah, ignore the POD. I was putting it in to start with, but I kept
changing interfaces and keeping the POD up to date was becoming a drag.
Eventually I just removed most of it on the premise that inaccurate
documentation is worse than no documentation.
It will have total coverage before I submit.
> +sub enable_driver
Is this an internal function? Test::Pod::Coverage will complain
unless internal function names are prefixed by underscore. (It
enforces POD documentation on all external functions).
No, this is an external function. However, there are a couple of
internal functions, so thanks for the tip.
> + # XXX: The following save should not be required, but
is
> + # If this save is omitted, by the time save is called just before
> + # mkinitrd, these changes will have been lost.
> + $g->aug_save();
We ought to ask David Lutterkort what's going on here, because I'm
quite sure that Augeas isn't supposed to behave like this.
> +sub disable_driver
[...]
> + $g->aug_rm($augeas);
Missing a path parameter.
Nope. The path parameter is $augeas ;)
> +# We can't rely on the index in the augeas path because it
will change if
> +# something has been inserted or removed before it.
> +# Look for the alias again in the same file which contained it on the first
> +# pass.
I'm sure that Augeas must provide a mechanism to name nodes. Ask
Augeas upstream about it?
Leave this one for the moment. I haven't tried removing this since I
twigged that I was mounting rw drives which had been added to qemu ro. I
could be that it works in the absence of underlying storage snafu.
> +sub add_kernel
> +{
> + my $self = shift;
> + my $kernel_arch = "i386"; # XXX: Need to get this from inspection!
Yup - it's on the TODO list, and does need to be fixed.
> +sub remap_block_devices
> +{
> + my $self = shift;
> + my %map = @_;
> +
> + my $g = $self->{g};
> +
> + # Iterate over fstab. Any entries with a spec in the the map, replace them
> + # with their mapped values
> + eval {
> + foreach my $spec ($g->aug_match('/etc/fstab/*/spec')) {
> + my $device = $g->aug_get($spec);
> + if(exists($map{$device})) {
> + $g->aug_set($spec, $map{$device});
> + }
> + }
> + };
> +
> + # Propagate augeas failure
> + die($@) if($@);
> +}
I'll ask a nasty question at this point: What happens if we are using
libguestfs with an IDE-based appliance (ie. devices named /dev/sd*),
but our target system will have virtio drivers (devices named
/dev/vd*)?
This function just takes a map passed to it from elsewhere. It should be
passed from HVTarget (did I miss that off the TODO list)? Anyway, the
nastiness is handled there.
We should probably use a neutral name instead of actual device names
in the internal structures ("HD(a,1)"), then map those on the way out.
Hopefully it won't matter too much since all modern distros use UUIDs
or labels, but beware of RHEL 3 guests.
In the grand scheme this isn't a huge issue. The additional code in
HVTarget will probably amount to a line or two. I'm not sure I'd bother.
[I checked all the other modules here but didn't have any
particular
comments about them].
> diff --git a/v2v/virt-v2v.pl b/v2v/virt-v2v.pl
[...]
> +# Create a squashfs filesystem containing all files given on the command line
I think you were right about ISO vs squashfs ... If you do use
squashfs, then note that RHEL 5 cannot mount it unless you use a
special mount_vfs call. See:
http://git.et.redhat.com/?p=libguestfs.git;a=commitdiff;h=9af502eff080179...
---
OK there's not as much duplicate code as I feared. In fact, your code
builds nicely on top of the inspection done in Lib.pm. How about we
leave Lib.pm alone?
Updating Lib.pm certainly isn't a priority because it's there and it
works. However, it's not pleasant or particularly understandable as an
API, or to extend. As a simple example, if there were a
GuestOS->getApplications() api, not only would the Red Hat and Debian
implementations have an obvious separation, but it could be populated
lazily, and it would be considerably more intuitive.
Matt
--
Matthew Booth, RHCA, RHCSS
Red Hat Engineering, Virtualisation Team
M: +44 (0)7977 267231
GPG ID: D33C3490
GPG FPR: 3733 612D 2D05 5458 8A8A 1600 3441 EA19 D33C 3490