[Libguestfs] [PATCH 4/4] inspector: Rewrite virt-inspector

[Matthew Booth]

I'm not entirely sure removing all the output methods except XML with no 
warning is an entirely good idea. This is an interface, after all, and 
it's being removed with no warning. Instead, I would recommend updating 
all the output methods except XML to display (to STDERR) a warning that 
this function is deprecated and will be removed in the next stable release.

That said, I have a couple of code comments below,

Matt

On 28/10/10 12:37, Richard W.M. Jones wrote:
> diff --git a/inspector/virt-inspector b/inspector/virt-inspector
> index 36fbfa9..3bd9069 100755
> --- a/inspector/virt-inspector
> +++ b/inspector/virt-inspector

> +sub output_applications
>   {
> -    foreach my $os (keys %$oses) {
> -        foreach my $kernel (@{$oses->{$os}->{kernels}}) {
> -            my $is_xen = $kernel->{version} =~ m/xen/;
> -            if ($is_xen) {
> -                print "xen_domU_kernel=yes\n";
> -                return;
> -            }
> +    local $_;
> +    my $root = shift;
> +    my $distro = shift;
> +
> +    # Based on the distro, take a guess at the package format
> +    # and package management.
> +    my ($package_format, $package_management);
> +    if ($distro eq "debian") {
> +        $package_format = "dpkg";
> +        $package_management = "apt";
> +    }
> +    elsif ($distro eq "fedora" || $distro =~/redhat/  || $distro =~/rhel/) {
> +        $package_format = "rpm";
> +        $package_management = "yum";
> +    }

This would identify RHEL 3 and RHEL 4 as using yum, which they don't. I 
suggest looking for /usr/bin/yum and /usr/bin/up2date instead.

> +    # else unknown.
> +
> +    $xml->dataElement (package_format =>  $package_format)
> +        if defined $package_format;
> +    $xml->dataElement (package_management =>  $package_management)
> +        if defined $package_management;
> +
> +    # Do we know how to get a list of applications?
> +    if (defined $package_format) {
> +        if ($package_format eq "rpm") {
> +            output_applications_rpm ($root);
>           }
> +        # else no we don't.
>       }
> -    print "xen_domU_kernel=no\n";
>   }
>
> -=item xen_pv_drivers=(yes|no)
> -
> -Answer C<yes>  if the guest has Xen paravirtualized drivers installed
> -(usually the kernel itself will be fully virtualized, but the PV
> -drivers have been installed by the administrator for performance
> -reasons).
> -
> -=cut
> -
> -sub output_query_xen_pv_drivers
> +sub output_applications_rpm
>   {
> -    foreach my $os (keys %$oses) {
> -        foreach my $kernel (@{$oses->{$os}->{kernels}}) {
> -            foreach my $module (@{$kernel->{modules}}) {
> -                if ($module =~ m/xen-/) {
> -                    print "xen_pv_drivers=yes\n";
> -                    return;
> -                }
> -            }
> +    local $_;
> +    my $root = shift;
> +
> +    # Previous virt-inspector ran the 'rpm' program from the guest.
> +    # This is insecure, and unnecessary because we can get the same
> +    # information directly from the RPM database.
> +
> +    my @applications;
> +
> +    eval {
> +        my ($fh, $filename) = tempfile (UNLINK =>  1);
> +        my $fddev = "/dev/fd/" . fileno ($fh);
> +        $g->download ("/var/lib/rpm/Name", $fddev);
> +        close $fh or die "close: $!";
> +
> +        # Read the database with the Berkeley DB dump tool.
> +        my $cmd = "db_dump -p '$filename'";
> +        open PIPE, "$cmd |" or die "close: $!";

The code should gracefully handle the case that db_dump returns an 
error, because the DB is corrupt.

> +        while (<PIPE>) {
> +            chomp;
> +            last if /^HEADER=END$/;
>           }
> -    }
> -    print "xen_pv_drivers=no\n";
> -}
> -
> -=item virtio_drivers=(yes|no)
> +        while (<PIPE>) {
> +            chomp;
> +            last if /^DATA=END$/;
>
> -Answer C<yes>  if the guest has virtio paravirtualized drivers
> -installed.  Virtio drivers are commonly used to improve the
> -performance of KVM.
> -
> -=cut
> -
> -sub output_query_virtio_drivers
> -{
> -    foreach my $os (keys %$oses) {
> -        foreach my $kernel (@{$oses->{$os}->{kernels}}) {
> -            foreach my $module (@{$kernel->{modules}}) {
> -                if ($module =~ m/virtio_/) {
> -                    print "virtio_drivers=yes\n";
> -                    return;
> -                }
> +            # First character on each data line is a space.
> +            if (length $_>  0&&  substr ($_, 0, 1) eq ' ') {

Weird spacing.

> +                $_ = substr ($_, 1);
>               }
> -        }
> -    }
> -    print "virtio_drivers=no\n";
> -}
> -
> -=item userspace_arch=(x86_64|...)
> -
> -Print the architecture of userspace.
> -
> -NB. For multi-boot VMs this can print several lines.
> -
> -=cut
> -
> -sub output_query_userspace_arch
> -{
> -    my %arches;
> -
> -    foreach my $os (keys %$oses) {
> -        $arches{$oses->{$os}->{arch}} = 1 if exists $oses->{$os}->{arch};
> -    }
> -
> -    foreach (sort keys %arches) {
> -        print "userspace_arch=$_\n";
> -    }
> -}
> -
> -=item kernel_arch=(x86_64|...)
> -
> -Print the architecture of the kernel.
> -
> -NB. For multi-boot VMs this can print several lines.
> -
> -=cut
> +            # Name should never contain non-printable chars.
> +            die "name contains non-printable chars" if /\\/;
> +            push @applications, $_;

This will fail to inspect a guest with a corrupt rpm database. The XML 
library will take care of escaping this properly, so you should just 
pass through whatever is received. The only potential exception could be 
a maximum length on the string.

>
> -sub output_query_kernel_arch
> -{
> -    my %arches;
> -
> -    foreach my $os (keys %$oses) {
> -        foreach my $kernel (@{$oses->{$os}->{kernels}}) {
> -            $arches{$kernel->{arch}} = 1 if exists $kernel->{arch};
> +            $_ =<PIPE>; # discard value
>           }
> -    }
> -
> -    foreach (sort keys %arches) {
> -        print "kernel_arch=$_\n";
> +        close PIPE or die "close: $!";
> +    };
> +    if (!$@&&  @applications>  0) {

Weird spacing.

Matt
-- 
Matthew Booth, RHCA, RHCSS
Red Hat Engineering, Virtualisation Team

GPG ID:  D33C3490
GPG FPR: 3733 612D 2D05 5458 8A8A 1600 3441 EA19 D33C 3490