[PATCH] Export inspect_linux_kernel in Lib.pm
--- perl/lib/Sys/Guestfs/Lib.pm | 38 +++++++++++++++++++++++++------------- 1 files changed, 25 insertions(+), 13 deletions(-) diff --git a/perl/lib/Sys/Guestfs/Lib.pm b/perl/lib/Sys/Guestfs/Lib.pm index 1f84bc6..dfa79af 100644 --- a/perl/lib/Sys/Guestfs/Lib.pm +++ b/perl/lib/Sys/Guestfs/Lib.pm @@ -66,7 +66,8 @@ use vars qw(@EXPORT_OK @ISA); @ISA = qw(Exporter); @EXPORT_OK = qw(open_guest get_partitions resolve_windows_path inspect_all_partitions inspect_partition - inspect_operating_systems mount_operating_system inspect_in_detail); + inspect_operating_systems mount_operating_system inspect_in_detail + inspect_linux_kernel); =head2 open_guest @@ -1551,10 +1552,19 @@ sub _check_for_kernels } $config{cmdline} = join(' ', @args) if(scalar(@args) > 0); - my $kernel = _inspect_linux_kernel($g, $os, "$path"); + my $kernel = + inspect_linux_kernel($g, $path, $os->{package_format}); # Check the kernel was recognised if(defined($kernel)) { + # Put this kernel on the top level kernel list + my $kernels = $os->{kernels}; + if(!defined($kernels)) { + $kernels = []; + $os->{kernels} = $kernels; + } + push(@$kernels, $kernel); + $config{kernel} = $kernel; # Look for an initrd entry @@ -1599,9 +1609,19 @@ sub _check_for_kernels } } -sub _inspect_linux_kernel +=head2 inspect_linux_kernel + + my $kernel_hash = inspect_linux_kernel($g, $vmlinuz_path, $package_format); + +inspect_linux_kernel returns a hash describing the target linux kernel. For the +contents of the hash, see the I<kernels> structure described under +L</inspect_in_detail>. + +=cut + +sub inspect_linux_kernel { - my ($g, $os, $path) = @_; + my ($g, $path, $package_format) = @_; my %kernel = (); @@ -1610,7 +1630,7 @@ sub _inspect_linux_kernel # If this is a packaged kernel, try to work out the name of the package # which installed it. This lets us know what to install to replace it with, # e.g. kernel, kernel-smp, kernel-hugemem, kernel-PAE - if($os->{package_format} eq "rpm") { + if($package_format eq "rpm") { my $package; eval { $package = $g->command(['rpm', '-qf', '--qf', '%{NAME}', $path]); }; @@ -1667,14 +1687,6 @@ sub _inspect_linux_kernel # of any kernel module. $kernel{arch} = file_architecture ($g, $any_module); - # Put this kernel on the top level kernel list - my $kernels = $os->{kernels}; - if(!defined($kernels)) { - $kernels = []; - $os->{kernels} = $kernels; - } - push(@$kernels, \%kernel); - return \%kernel; } -- 1.6.2.5
On Wed, Aug 19, 2009 at 12:54:41PM +0100, Matthew Booth wrote:
--- perl/lib/Sys/Guestfs/Lib.pm | 38 +++++++++++++++++++++++++------------- 1 files changed, 25 insertions(+), 13 deletions(-)
ACK. Rich. -- Richard Jones, Emerging Technologies, Red Hat http://et.redhat.com/~rjones virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://et.redhat.com/~rjones/virt-top
Matthew Booth wrote:
diff --git a/perl/lib/Sys/Guestfs/Lib.pm b/perl/lib/Sys/Guestfs/Lib.pm ... @@ -1551,10 +1552,19 @@ sub _check_for_kernels ... # Check the kernel was recognised if(defined($kernel)) { + # Put this kernel on the top level kernel list + my $kernels = $os->{kernels}; + if(!defined($kernels)) { + $kernels = []; + $os->{kernels} = $kernels; + } + push(@$kernels, $kernel); +
Hi Matt, It took me too long to see what was being done above, so I rewrote it in a way that I found easier to read: How about this? diff --git a/perl/lib/Sys/Guestfs/Lib.pm b/perl/lib/Sys/Guestfs/Lib.pm index dfa79af..2acdec6 100644 --- a/perl/lib/Sys/Guestfs/Lib.pm +++ b/perl/lib/Sys/Guestfs/Lib.pm @@ -1558,12 +1558,8 @@ sub _check_for_kernels # Check the kernel was recognised if(defined($kernel)) { # Put this kernel on the top level kernel list - my $kernels = $os->{kernels}; - if(!defined($kernels)) { - $kernels = []; - $os->{kernels} = $kernels; - } - push(@$kernels, $kernel); + $os->{kernels} ||= []; + push(@{$os->{kernels}}, $kernel); $config{kernel} = $kernel; The ||= operator may look odd, but it's another of these idiom things. Once you see it a few times, then "get it", and start using it, you'll never go back. Which would you prefer to read/maintain? This: # Ensure the array-ref is initialized. if (!defined $some_long_name->[$some_complicated_expression]) { $some_long_name->[$some_complicated_expression] = []; } # Append to it. ... or this: # Ensure the array-ref is initialized. $some_long_name->[$some_complicated_expression] ||= []; # Append to it. ...
On 19/08/09 15:18, Jim Meyering wrote:
Matthew Booth wrote:
diff --git a/perl/lib/Sys/Guestfs/Lib.pm b/perl/lib/Sys/Guestfs/Lib.pm ... @@ -1551,10 +1552,19 @@ sub _check_for_kernels ... # Check the kernel was recognised if(defined($kernel)) { + # Put this kernel on the top level kernel list + my $kernels = $os->{kernels}; + if(!defined($kernels)) { + $kernels = []; + $os->{kernels} = $kernels; + } + push(@$kernels, $kernel); +
Hi Matt,
It took me too long to see what was being done above, so I rewrote it in a way that I found easier to read:
How about this?
diff --git a/perl/lib/Sys/Guestfs/Lib.pm b/perl/lib/Sys/Guestfs/Lib.pm index dfa79af..2acdec6 100644 --- a/perl/lib/Sys/Guestfs/Lib.pm +++ b/perl/lib/Sys/Guestfs/Lib.pm @@ -1558,12 +1558,8 @@ sub _check_for_kernels # Check the kernel was recognised if(defined($kernel)) { # Put this kernel on the top level kernel list - my $kernels = $os->{kernels}; - if(!defined($kernels)) { - $kernels = []; - $os->{kernels} = $kernels; - } - push(@$kernels, $kernel); + $os->{kernels} ||= []; + push(@{$os->{kernels}}, $kernel);
$config{kernel} = $kernel;
The ||= operator may look odd, but it's another of these idiom things. Once you see it a few times, then "get it", and start using it, you'll never go back.
Which would you prefer to read/maintain? This:
# Ensure the array-ref is initialized. if (!defined $some_long_name->[$some_complicated_expression]) { $some_long_name->[$some_complicated_expression] = []; }
# Append to it. ...
or this:
# Ensure the array-ref is initialized. $some_long_name->[$some_complicated_expression] ||= [];
# Append to it. ...
I haven't come across that idiom, and I like it. However, I've already pushed this patch now and I'm not sure it merits its own revision given the number of other places I could make the same change. I suggest making this change over time as code is touched. Matt -- Matthew Booth, RHCA, RHCSS Red Hat Engineering, Virtualisation Team M: +44 (0)7977 267231 GPG ID: D33C3490 GPG FPR: 3733 612D 2D05 5458 8A8A 1600 3441 EA19 D33C 3490
Matthew Booth wrote:
I haven't come across that idiom, and I like it. However, I've already pushed this patch now and I'm not sure it merits its own revision given the number of other places I could make the same change.
I suggest making this change over time as code is touched.
IMHO, there's no harm in burning a cset for a change like this, assuming you like it. No pressure, though.
--- perl/lib/Sys/Guestfs/Lib.pm | 17 ++++------------- 1 files changed, 4 insertions(+), 13 deletions(-) diff --git a/perl/lib/Sys/Guestfs/Lib.pm b/perl/lib/Sys/Guestfs/Lib.pm index dfa79af..96ada0d 100644 --- a/perl/lib/Sys/Guestfs/Lib.pm +++ b/perl/lib/Sys/Guestfs/Lib.pm @@ -1558,12 +1558,8 @@ sub _check_for_kernels # Check the kernel was recognised if(defined($kernel)) { # Put this kernel on the top level kernel list - my $kernels = $os->{kernels}; - if(!defined($kernels)) { - $kernels = []; - $os->{kernels} = $kernels; - } - push(@$kernels, $kernel); + $os->{kernels} ||= []; + push(@{$os->{kernels}}, $kernel); $config{kernel} = $kernel; @@ -1766,13 +1762,8 @@ sub _inspect_initrd } # Add to the top level initrd_modules entry - my $initrd_modules = $os->{initrd_modules}; - if(!defined($initrd_modules)) { - $initrd_modules = {}; - $os->{initrd_modules} = $initrd_modules; - } - - $initrd_modules->{$version} = \@modules; + $os->{initrd_modules} ||= {}; + $os->{initrd_modules}->{$version} = \@modules; return \@modules; } -- 1.6.2.5
Matthew Booth wrote:
diff --git a/perl/lib/Sys/Guestfs/Lib.pm b/perl/lib/Sys/Guestfs/Lib.pm index dfa79af..96ada0d 100644 --- a/perl/lib/Sys/Guestfs/Lib.pm +++ b/perl/lib/Sys/Guestfs/Lib.pm @@ -1558,12 +1558,8 @@ sub _check_for_kernels # Check the kernel was recognised if(defined($kernel)) { # Put this kernel on the top level kernel list - my $kernels = $os->{kernels}; - if(!defined($kernels)) { - $kernels = []; - $os->{kernels} = $kernels; - } - push(@$kernels, $kernel); + $os->{kernels} ||= []; + push(@{$os->{kernels}}, $kernel);
$config{kernel} = $kernel;
@@ -1766,13 +1762,8 @@ sub _inspect_initrd }
# Add to the top level initrd_modules entry - my $initrd_modules = $os->{initrd_modules}; - if(!defined($initrd_modules)) { - $initrd_modules = {}; - $os->{initrd_modules} = $initrd_modules; - } - - $initrd_modules->{$version} = \@modules; + $os->{initrd_modules} ||= {}; + $os->{initrd_modules}->{$version} = \@modules;
Good clean-up. ACK.
participants (3)
-
Jim Meyering -
Matthew Booth -
Richard W.M. Jones