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