[Libguestfs] [PATCH] Initial drop of virt-v2v

[Matthew Booth]

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=9af502eff08017941a58ad676d0cbb867f83a341
>
> 	---
>
> 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