[Libguestfs] [PATCH 3/4] Add SUSE converter
Matthew Booth
mbooth at redhat.com
Thu Sep 26 11:33:18 UTC 2013
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
More information about the Libguestfs
mailing list