Re: [Libguestfs] [PATCH 3/4] tools: Use C API for inspection

On Thu, Oct 28, 2010 at 01:52:24PM +0100, Matthew Booth wrote: > On 28/10/10 12:37, Richard W.M. Jones wrote: >> > From 6e19d032d0d28e62f38223b6cc05dc36d9352add Mon Sep 17 00:00:00 2001 >> From: Richard W.M. Jones&lt;rjones(a)redhat.com&gt; >> Date: Wed, 27 Oct 2010 16:06:11 +0100 >> Subject: [PATCH 3/4] tools: Use C API for inspection (RHBZ#642930). >> >> Update the following tools to use the C API for inspection: >> >> - virt-cat >> - virt-edit >> - virt-ls >> - virt-tar >> - virt-win-reg >> >> None of the tools in the tools/ directory now use the deprecated >> Perl inspection APIs. >> --- >> tools/virt-cat | 37 +++++++++++++++++-------------------- >> tools/virt-edit | 35 ++++++++++++++++------------------- >> tools/virt-ls | 39 ++++++++++++++++++--------------------- >> tools/virt-tar | 38 ++++++++++++++++++-------------------- >> tools/virt-win-reg | 39 ++++++++++++++++++--------------------- >> 5 files changed, 87 insertions(+), 101 deletions(-) >> >> diff --git a/tools/virt-cat b/tools/virt-cat >> index 66806a1..546e85c 100755 >> --- a/tools/virt-cat >> +++ b/tools/virt-cat >> @@ -157,22 +156,20 @@ if ($uri) { >> >> $g->launch (); >> >> -# List of possible filesystems. >> -my @partitions = get_partitions ($g); >> - >> -# Now query each one to build up a picture of what's in it. >> -my %fses = >> - inspect_all_partitions ($g, \@partitions, >> - use_windows_registry => 0); >> - >> -my $oses = inspect_operating_systems ($g, \%fses); >> - >> -my @roots = keys %$oses; >> -die __"multiboot operating systems are not supported by virt-cat" if @roots> 1; >> -my $root_dev = $roots[0]; >> - >> -my $os = $oses->{$root_dev}; >> -mount_operating_system ($g, $os); >> +my @roots = $g->inspect_os (); >> +if (@roots == 0) { >> + die __x("{prog}: No operating system could be detected inside this disk image.\n\nThis may be because the file is not a disk image, or is not a virtual machine\nimage, or because the OS type is not understood by libguestfs.\n\nIf you feel this is an error, please file a bug report including as much\ninformation about the disk image as possible.\n", >> + prog => basename ($0)); >> +} >> +if (@roots> 1) { >> + die __x("{prog}: multiboot operating systems are not supported.\n", >> + prog => basename ($0)) >> +} >> +my %fses = $g->inspect_get_mountpoints ($roots[0]); >> +my @fses = sort { length $a<=> length $b } keys %fses; >> +foreach (@fses) { >> + $g->mount_ro ($fses{$_}, $_); >> +} >> >> # Allow this to fail in case eg. the file does not exist. >> # NB:https://bugzilla.redhat.com/show_bug.cgi?id=501888 > > This is pretty long for boiler plate code which will be used in all > the virt tools, including virt-v2v. This should live in a high-level > library function which: > > 1. Inspects a guest. > 2. Mounts all its storage, optionally read-only. That's how I did it first time, but: > It wouldn't necessarily need to form part of a published API this is the problem. If it's in Sys::Guestfs::Lib it's pretty much a published API.

Also the boilerplate isn't quite the same each time, particularly w.r.t whether we allow multiple roots (virt-inspector) or require writable mounts (eg. virt-edit).

I don't think there's a good answer here. At least the error messages are the same so they only have to be translated once.