[pve-devel] [PATCH qemu-server 1/2] Add DiskImport module to import external disk images into a VM

[Thomas Lamprecht]

some comments inline:

On 04/25/2017 10:56 AM, Emmanuel Kasper wrote:
> Each disk passed as paramater is add as 'unused[n]' in the vm.conf
> (the default) or ide[n]|scsi[n]|sata[n]
> We rely on qemu-img(1) convert heuristics to detect the file type,
> as this works in most case.
> ---
>   PVE/QemuServer/DiskImport.pm | 93 ++++++++++++++++++++++++++++++++++++++++++++
>   PVE/QemuServer/Makefile      |  1 +
>   2 files changed, 94 insertions(+)
>   create mode 100755 PVE/QemuServer/DiskImport.pm
>
> diff --git a/PVE/QemuServer/DiskImport.pm b/PVE/QemuServer/DiskImport.pm
> new file mode 100755
> index 0000000..2d210ae
> --- /dev/null
> +++ b/PVE/QemuServer/DiskImport.pm
> @@ -0,0 +1,93 @@
> +package PVE::QemuServer::DiskImport;
> +
> +use strict;
> +use warnings;
> +use Data::Dumper;
> +
> +use PVE::Storage;
> +use PVE::Storage::Plugin;
> +use PVE::QemuServer;
> +use PVE::Tools 'run_command';
> +
> +# given a hash reference with the following structure,
> +# { src_path => $path_to_file,
> +#   vmid => target_vmid,
> +#   storage_id => $storage_id,
> +#   disk_entry => $disk_entry, # leave this undef to create an 'unused' volume
> +#   debug => 0 }
> +# this sub will import an external disk image to a PVE storage
> +# and create a disk_entry key (ide0 | unused0) pointing to the created volume
> +sub do_import {
> +    my ($args) = @_;
IMHO a "normal" parameter signature, like:

my ($vmid, $storage_id, ...) = @_;

would be a little bit better.

As a) it's easier to make unnoticed typos with hash keys (Perl doesn't 
complain on them, but it complains on unknown variable names)
and b) is most times more related to our coding style.

If you expect that the signature of this method could grow quite a bit 
I'd suggest a mixed approach, i.e. use normal variables for the required 
params and a hash for the optional arguments, i.e.:
my ($vmid, $storage_id, %params) = @_;

we do this on some parts in the code already. The method could then be 
called like:

do_import($vmid, $sid, debug => 1, future_option => 'foo');


> +    my $dst = {}; # parameters that we will pass to qemu-img

nit pick: i find the comment a little misleading as only one value of 
this hash gets passed to qemu-img ($dst->{path})

> +
> +    # get the needed size from  source disk
> +    my ($virtual_src_size, $src_format, $used, $parent) = PVE::Storage::Plugin::file_size_info($args->{src_path});
> +
> +    # allocate an image of the needed size
> +    my $storecfg = PVE::Storage::config();
> +    $dst->{volid} = PVE::Storage::vdisk_alloc($storecfg,
> +					    $args->{storage_id}, $args->{vmid}, undef, undef, $virtual_src_size / 1024);
> +
> +    # get target image's path, whether it's possible to sparseinit, and target format
> +    $dst->{path} = PVE::Storage::path($storecfg, $dst->{volid});
> +    $dst->{sparseinit} = PVE::Storage::volume_has_feature($storecfg, 'sparseinit', $dst->{volid});
> +    $dst->{format} = PVE::QemuServer::qemu_img_format($storecfg, $args->{storage_id});
> +    $dst->{size} = $virtual_src_size;
> +
> +    warn "\$args: ", Dumper($args), "\$dst->{volid}: $dst->{volid}", "\$dst: ", Dumper($dst) if $args->{debug};
> +
> +    # qemu-img convert does the hard job
> +    # we don't attempt to guest filetypes ourselves
> +    my $convert_command = ['qemu-img', 'convert', $args->{src_path}, '-p', '-n'];
> +    if ($dst->{has_sparseinit}) {

'has_sparseinit' does not exist in $dst, i guess 'sparseinit' was meant.

> +	push @$convert_command, "zeroinit:$dst->{path}";
> +    } else {
> +	push @$convert_command, $dst->{path};
> +    }
> +
> +    my $config_key;
> +    eval {
> +	# trap interrupts so we have a chance to clean up
> +	local $SIG{INT} = $SIG{TERM} = $SIG{QUIT} = $SIG{HUP} = $SIG{PIPE} = sub {
> +	    die "interrupted by signal\n";
> +	};
> +	PVE::Storage::activate_volumes($storecfg, [$dst->{volid}]);
> +	run_command($convert_command);
> +	PVE::Storage::deactivate_volumes($storecfg, [$dst->{volid}]);
> +	$config_key = __create_disk_entry($storecfg, $args->{vmid}, $args->{disk_entry}, $dst);
> +    };
> +    my $err = $@;
> +    if ($err) {
> +	PVE::Storage::vdisk_free($storecfg, $dst->{volid}) if PVE::Storage::path($storecfg, $dst->{volid});
> +	die $err;
> +    }
> +
> +    return $config_key;
> +}
> +
> +sub __create_disk_entry {
> +    my ($storecfg, $vmid, $disk_entry, $dst) = @_;
> +    if (PVE::QemuServer::vm_is_volid_owner($storecfg, $vmid, $dst->{volid})) {
> +	my $vm_conf = PVE::QemuConfig->load_config($vmid);
> +
> +	my $create_active_disk_entry = sub {
> +	    if (PVE::QemuServer::is_valid_disk_entryname($disk_entry)) {
> +		my $vm_conf->{$disk_entry} = PVE::QemuServer::print_disk_entry($vmid, { file => $dst->{volid}, size => $dst->{size} });

s/disk_entry/drive/ in the function names (is_valid_drivename() and 
print_drive()).

> +		PVE::QemuConfig->write_config($vmid, $vm_conf);
> +		return $disk_entry;
> +	    }
> +	    return undef;
> +	};
> +
> +	my $create_unused_disk_entry =  sub {
> +	    my $disk_key = PVE::QemuConfig->add_unused_volume($vm_conf, $dst->{volid});
> +	    PVE::QemuConfig->write_config($vmid, $vm_conf);
> +	    return $disk_key;
> +	};
> +
> +	return PVE::QemuConfig->lock_config($vmid, $disk_entry ? $create_active_disk_entry : $create_unused_disk_entry);
> +    }
> +}
> +
> +1;
> diff --git a/PVE/QemuServer/Makefile b/PVE/QemuServer/Makefile
> index 06617c5..b61a99a 100644
> --- a/PVE/QemuServer/Makefile
> +++ b/PVE/QemuServer/Makefile
> @@ -3,3 +3,4 @@ install:
>   	install -D -m 0644 PCI.pm ${DESTDIR}${PERLDIR}/PVE/QemuServer/PCI.pm
>   	install -D -m 0644 USB.pm ${DESTDIR}${PERLDIR}/PVE/QemuServer/USB.pm
>   	install -D -m 0644 Memory.pm ${DESTDIR}${PERLDIR}/PVE/QemuServer/Memory.pm
> +	install -D -m 0644 DiskImport.pm ${DESTDIR}${PERLDIR}/PVE/QemuServer/DiskImport.pm