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

Matthew Booth
2013-09-26T11:33:18Z

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