[Libguestfs] [PATCH] virt-v2v: Convert RedHat.pm to Linux.pm - for SUSE support

[Matthew Booth]

This approach looks great. I think there are a couple of bits to look at
(see below), but probably not much.

Matt

On Wed, 2013-10-02 at 23:46 -0600, Mike Latimer wrote:
> This is a proposed patch which changes the RedHat.pm converter to Linux.pm,
> and adds support for SUSE guest conversion. This is first approach recommended
> by Matt Booth in:
> 
> https://www.redhat.com/archives/libguestfs/2013-September/msg00076.html
> 
> Some aspects of this patch still need additional testing, and a couple of
> changes are not foolproof (such as the lack of grub2 support in the menu.lst
> changes in _remap_block_devices). However, it is complete enough that I'd
> like some feedback before continuing.
> 
> Note - This patch alone is not enough to support SUSE guests, as changes to
> virt-v2v.db are also required. After these changes are flushed out, I'll post
> all the patches in one thread.
> 
> -Mike

N.B. I've cut the original below and replaced it with the output of git
show, which deals with the rename sensibly. The patch is identical,
though.


> diff --git a/lib/Sys/VirtConvert/Converter/RedHat.pm b/lib/Sys/VirtConvert/Converter/Linux.pm
> similarity index 79%
> rename from lib/Sys/VirtConvert/Converter/RedHat.pm
> rename to lib/Sys/VirtConvert/Converter/Linux.pm
> index 612ab2e..f3d13df 100644
> --- a/lib/Sys/VirtConvert/Converter/RedHat.pm
> +++ b/lib/Sys/VirtConvert/Converter/Linux.pm



> @@ -500,7 +584,7 @@ sub convert_efi
>      eval { $g->aug_save(); };
>      augeas_error($g, $@) if $@;
>  
> -    # Install grub2 in the BIOS boot partition. This overwrites the previous
> +    # Install grub2 in the BIOS beot partition. This overwrites the previous

Typo

>      # contents of the EFI boot partition.
>      $g->command(['grub2-install', $device]);


> @@ -1002,8 +1101,9 @@ sub _configure_kernel
>          last;
>      }
>  
> -    # There should be an installed virtio capable kernel if virtio was installed
> -    die("virtio configured, but no virtio kernel found")
> +    # Warn if there is no installed virtio capable kernel. It is safe to
> +    # continue and attempt to install a virtio kernel next.
> +    logmsg NOTICE, __x('virtio capable guest, but no virtio kernel found.')
>          if ($virtio && !defined($boot_kernel));

No. This indicates an error in the program rather than something the
user can fix. This is also why it's a plain die with no translation.

>  
>      # If none of the installed kernels are appropriate, install a new one


> @@ -1055,7 +1155,7 @@ sub _configure_kernel
>                      unless defined($boot_kernel);
>              } else {
>                  v2vdie __x('Failed to find a {name} package to install',
> -                           name => "kernel_pkg.$kernel_arch");
> +                           name => "$kernel_pkg.$kernel_arch");

Thanks :)


> @@ -1631,14 +1752,38 @@ sub _install_any
>      # If we're installing a kernel, check which kernels are there first
>      my @k_before = $g->glob_expand('/boot/vmlinuz-*') if defined($kernel);
>  
> +    # If a SLES11 kernel is being added, add -base kernel
> +    if (defined($kernel)) {
> +        if (_is_suse_family($g, $root) &&
> +            ($g->inspect_get_major_version($root) == 11)) {
> +            my $tmp;
> +            push(@$tmp, $kernel);
> +            $tmp->[1][0] = $tmp->[0][0].'-base';
> +            for my $count (1..4) {
> +                if (defined($tmp->[0][$count])) {
> +                    $tmp->[1][$count] = $tmp->[0][$count];
> +                }
> +            }
> +            $kernel = $tmp;
> +        }
> +    }

For sanity, I think we need $kernel to be the same data structure in
all cases. Lets either unconditionally convert $kernel into a list (of
lists), or see of -base can be added to the @install list or something
like that. Bear in mind, though, that in the former case you'd also
have to update _install_yum and _install_up2date.

(I haven't bothered commenting on all the hunks this change would also affect below.)


> @@ -2235,6 +2541,10 @@ sub _remap_block_devices
>      # Fedora has used libata since FC7, which is long out of support. We assume
>      # that all Fedora distributions in use use libata.
>  
> +    # Create a hash to track any hd->sd conversions, as any references to
> +    # hd devices (in fstab, menu.lst, etc) will need to be converted later.
> +    my %idemap;
> +
>      if ($libata) {
>          # If there are any IDE devices, the guest will have named these sdX
>          # after any SCSI devices. i.e. If we have disks hda, hdb, sda and sdb,
> @@ -2247,10 +2557,15 @@ sub _remap_block_devices
>          # this is a weird and somewhat unlikely situation I'm going with SCSI
>          # first until we have a more comprehensive solution.
>  
> +        my $idedev;
>          my @newdevices;
>          my $suffix = 'a';
>          foreach my $device (@devices) {
> -            $device = 'sd'.$suffix++ if ($device =~ /(?:h|s)d[a-z]+/);
> +            if ($device =~ /(?:h|s)d[a-z]+/) {
> +                $idedev = $device;
> +                $device = 'sd'.$suffix++;
> +                $idemap{$device} = $idedev if ($device ne $idedev);
> +            }
>              push(@newdevices, $device);
>          }
>          @devices = @newdevices;
> @@ -2286,12 +2601,21 @@ sub _remap_block_devices
>              $map{'xvd'.$1} = $mapped;
>          }
>          $map{$device} = $mapped;
> +        # Also add a mapping for previously saved hd->sd conversions
> +        if (defined($idemap{$device})) {
> +            my $idedevice;
> +            $idedevice = $idemap{$device};
> +            $map{$idedevice} = $mapped;
> +        }
> +

I've had a change to think about this change properly, and I'm pretty
sure it's not required. The reason is that the only place %idemap is
modified is in the section guarded:

if ($libata) {
...
}

If the guest is using libata it will not have any references to
/dev/hdX devices, as IDE devices are presented as SCSI devices. That's
a NAK to this bit of the patch. Instead, you need to work out when SLES
switched to libata and set $libata accordingly above.