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

> > + # 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.

> > + 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.

> > + 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.

> > + 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.

> > +# 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.