[Libguestfs] [PATCH 4/5] New tool: virt-tar

[Matthew Booth]

On 19/10/09 12:24, Richard W.M. Jones wrote:
> +sub set_mode_x
> +{
> +    die __"virt-tar: extract/upload mode specified twice on the command line"
> +	if $mode;
> +    $mode = "x";
> +}
> +
> +sub set_mode_u
> +{
> +    die __"virt-tar: extract/upload mode specified twice on the command line"
> +	if $mode;
> +    $mode = "u";
> +}

I know we've done this in plenty of other places, but die() really isn't 
a very nice user interface. I would change both of these to:

if($mode) {
   print STDERR __"msg";
   exit(1);
}

Even better, create an error reporting function which also prepends 
'virt-tar: ' so this string doesn't show up repeatedly for translation.

> +Getopt::Long::Configure ("bundling");
> +GetOptions ("help|?" =>  \$help,
> +            "version" =>  \$version,
> +            "connect|c=s" =>  \$uri,
> +	    "extract|download|x" =>  \&set_mode_x,
> +	    "upload|u" =>  \&set_mode_u,
> +	    "gzip|z" =>  \$gzip,
> +    ) or pod2usage (2);

Mixed tabs and spaces.

> +pod2usage (1) if $help;
> +if ($version) {
> +    my $g = Sys::Guestfs->new ();
> +    my %h = $g->version ();
> +    print "$h{major}.$h{minor}.$h{release}$h{extra}\n";
> +    exit
> +}
> +
> +pod2usage (__"virt-tar: no image, VM names, directory or filename given")
> +    if @ARGV<= 2;
> +
> +# 'pop' reads arguments right to left.
> +my $tarball = pop @ARGV;
> +my $directory = pop @ARGV;
> +
> +die __"virt-tar: either -x or -u must be specified on the command line"
> +    unless $mode;
> +
> +my @args = (\@ARGV);
> +push @args, address =>  $uri if $uri;
> +push @args, rw =>  1 if $mode eq "u";
> +
> +my $g = open_guest (@args);
> +$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 __"no root device found in this operating system image" if @roots == 0;
> +die __"multiboot operating systems are not supported by virt-edit" if @roots>  1;

It's virt-tar, not virt-edit ;)

> +my $root_dev = $roots[0];
> +
> +my $os = $oses->{$root_dev};
> +mount_operating_system ($g, $os, $mode eq "u" ? 0 : 1);

The cut/paste above suggests we could do with a library function for this.

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