[Libguestfs] [PATCH 3/4] Add SUSE converter

[Matthew Booth]

On Wed, 2013-09-25 at 12:21 -0600, Mike Latimer wrote:
> > > +    # Look for an EFI configuration
> > > +    foreach my $cfg ($g->glob_expand('/boot/*efi/*/grub.conf')) {
> > 
> > I noticed you've changed this. Out of curiosity, where does SUSE mount
> > it?
> 
> Typically, it's mounted at /boot/efi, but the next level efi is lowercase. 
> However, the minor efi changes are what I inherited (well, that and the 
> copyright changes), so I have some more investigation to do here.
> 
> > > +    # Nothing to do if the kernel already has a grub entry
> > > +    return if $g->aug_match("/files$grub_conf/title/kernel".
> > > +                            "[. = '$grub_path']") ne '';
> > 
> > I notice this is a change from RedHat.pm. Why did you add the "ne ''"?
> > Augeas will only return kernel entries, and I would expect a blank
> > kernel entry to be a parse error, hence this would be redundant.
> 
> In my testing (and according to RHBZ#974441), a null string is returned, and 
> the following error is encountered:
> 
> Argument "/files/boot/grub/menu.lst/title[1]/kernel" isn't numeric in numeric 
> gt (>) at /usr/share/perl5/vendor_perl/Sys/VirtConvert/Converter/RedHat.pm 
> line 206.
> 
> This causes the routine to continue, and an unecessary entry is added to 
> menu.lst.

That's a bug in v2v, not in augeas. The problem is that the return of
aug_match is a list, but it's not in scalar context so it isn't numeric.
The fix (now upstream) is to force it into scalar context.

> > > +        foreach my $path ($g->aug_match('/files'.$xorg.'/Device/Driver'))
> > > { +            $g->aug_set($path, 'cirrus');
> > 
> > I've changed the default to qxl now.
> 
> SUSE still uses cirrus.

Yeah, we discussed this. We decided that qxl is still a better default
than cirrus because it has an excellent VGA fallback mode. In practise I
have found that if the client doesn't have drivers for either, the
performance of qxl is still better.

This would be a matter for you, though.

> > > +    logmsg NOTICE, __x('Falling back to local virt-v2v repo:');
> > > +    foreach my $pkg (@rpms) {
> > > +        (my $pkgname = $pkg) =~ s/(.*\/)//;
> > 
> > basename would have saved me a moment of squinting :)
> 
> Yes, but would it have been as fun?  ;)  Using basename would require 
> File::Basename, and I wasn't sure if that would be acceptable.

The additional dep is fine.

> > > +    eval { $g->command(['rpm', $update == 1 ? '-U' : '-i', @rpms]) };
> > > +    if ($@) {
> > > +        my $error = $@;
> > > +        # Try to isolate error to a meaningful string
> > > +        $error =~ /^(command.*error:)\s+(.*:)\s+(.*)\s(at \/usr\/.*)/;
> > > +        logmsg WARN, __x('Failed to install packages. '.
> > > +                         'Error was: {error}', error => "$2 $3");
> > 
> > What error message is this trying to parse? We should avoid trying to
> > parse error message wherever possible as they are notorious for
> > differing between versions.
> 
> The warning I was trying to isolate is:
> 
> command: command: error: Failed dependencies:
>         kernel-default-base_x86_64 = 3.0.76-0.11.1 is needed by kernel-
> default-3.0.76-0.11.1.x86_64 at 
> /usr/lib/perl5/vendor_perl/5.16.2/Sys/VirtConvert/GuestfsHandle.pm line 198.
>  at /usr/lib/perl5/vendor_perl/5.16.2/Sys/VirtConvert/Converter/SUSE.pm line 
> 1885.
> 
> This error should only be seen if someone has misconfigured their kernel and 
> kernel-base entries in the virt-v2v .conf (or didn't copy the rpms down 
> properly), but my goal was to strip out the 'command: command: error:' and 'at 
> /usr/lib....' parts of the error, as they don't help. Is a better approach to 
> leave the entire string in, or is there a more acceptable way to clean it up?
> 
> > Also, this should almost certainly die (with v2vdie) rather than be a
> > warning.

Ok. I'd strip out 'command: command: error: ', but the rest is an
artifact of how this error is raised. If you replace it with a v2vdie,
they should disappear in any case.

This is a common fix with RedHat.pm.

> > > +# The next two functions are a temporary workaround to bnc#836521. The
> > > actual +# fix is in a new perl-Bootloader, which will likely not be on
> > > most guests. +sub _modify_perlBootloader
> > 
> > Welcome to being forced to work around bugs which were fixed long ago ;)
> 
> Thanks! It's an exciting place to be ;)
> 
> Thanks again for all the comments. I'll clean this up and post a rev2 next 
> week.

Thanks,

Matt