Re: [Libguestfs] [PATCH 1/7] Push $desc creation into Sys::VirtConvert::Converter->convert

On Tue, Apr 26, 2011 at 05:03:46PM +0100, Matthew Booth wrote: > @@ -934,21 +939,23 @@ sub _get_application_owner > > sub _unconfigure_hv > { > - my ($g, $desc) = @_; > + my ($g, $root, $desc) = @_; > > - _unconfigure_xen($g, $desc); > - _unconfigure_vmware($g, $desc); > + my @apps = $g->inspect_list_applications($root); I can't tell without looking at the full context, but this might not be safe. inspect_list_applications will only work if the mountpoints are all mounted up. That's because this one call can read files out of the filesystem, where as all the inspect_get_* calls just return data that was cached in the handle (by inspect_os).

Note the comment in the removed code below which says just about the same thing: > - my $root_dev = $roots[0]; > - > - # Mount up the disks. > - my %fses = $g->inspect_get_mountpoints ($root_dev); > - my @fses = sort { length $a<=> length $b } keys %fses; > - foreach (@fses) { > - eval { $g->mount_options ("", $fses{$_}, $_) }; > - print __x("{e} (ignored)\n", e => $@) if $@; > - } > - > - # Construct the "$desc" hashref which contains the main features > - # found by inspection. > - my %desc; > - > - $desc{root_device} = $root_dev; > - > - $desc{os} = $g->inspect_get_type ($root_dev); > - $desc{distro} = $g->inspect_get_distro ($root_dev); > - $desc{product_name} = $g->inspect_get_product_name ($root_dev); > - $desc{product_variant} = $g->inspect_get_product_variant ($root_dev); > - $desc{major_version} = $g->inspect_get_major_version ($root_dev); > - $desc{minor_version} = $g->inspect_get_minor_version ($root_dev); > - $desc{arch} = $g->inspect_get_arch ($root_dev); > - > - # Notes: > - # (1) Filesystems have to be mounted for this to work. Do not > - # move this code over the filesystem mounting code above. > - # (2) For RPM-based distros, new libguestfs inspection code > - # is only able to populate the 'app_name' field (old Perl code > - # populated a lot more). Fortunately this is the only field > - # that the code currently uses. > - my @apps = $g->inspect_list_applications ($root_dev); > - $desc{apps} = \@apps; > - > - return \%desc; > + return $roots[0]; > } References: http://libguestfs.org/guestfs.3.html#inspection http://libguestfs.org/guestfs.3.html#guestfs_inspect_list_applications The rest seems all fine. I'd be a lot happier if our language/ compiler was strongly type-checking all of these changes though ...