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

[Fabian Grünbichler]

comments inline

On Mon, May 22, 2017 at 10:34:46AM +0200, 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/ImportDisk.pm | 87 ++++++++++++++++++++++++++++++++++++++++++++
>  PVE/QemuServer/Makefile      |  1 +
>  2 files changed, 88 insertions(+)
>  create mode 100755 PVE/QemuServer/ImportDisk.pm
> 
> diff --git a/PVE/QemuServer/ImportDisk.pm b/PVE/QemuServer/ImportDisk.pm
> new file mode 100755
> index 0000000..adab699
> --- /dev/null
> +++ b/PVE/QemuServer/ImportDisk.pm
> @@ -0,0 +1,87 @@
> +package PVE::QemuServer::ImportDisk;
> +
> +use strict;
> +use warnings;
> +
> +use PVE::Storage;
> +use PVE::QemuServer;
> +use PVE::Tools qw(run_command extract_param);
> +
> +# imports an external disk image to an existing VM
> +# and creates by default a drive entry unused[n] pointing to the created volume
> +# $optional->{drive_name} may be used to specify ide0, scsi1, etc ...
> +# $optional->{format} may be used to specify qcow2, raw, etc ...
> +sub do_import {
> +    my ($src_path, $vmid, $storage_id, $optional) = @_;
> +
> +    my $drive_name = extract_param($optional, 'drive_name');
> +    my $format = extract_param($optional, 'format');
> +    my $debug = extract_param($optional, 'debug');
> +    if ($drive_name && !(PVE::QemuServer::is_valid_drivename($drive_name))) {
> +	die "invalid drive name: $drive_name\n";
> +    }
> +
> +    # get the needed size from  source disk
> +    my $src_size = PVE::Storage::file_size_info($src_path);
> +
> +    # get target format, target image's path, and whether it's possible to sparseinit
> +    my $storecfg = PVE::Storage::config();
> +    my $dst_format = PVE::QemuServer::resolve_dst_disk_format($storecfg,
> +	$storage_id, undef, $format);
> +    warn "format : $dst_format\n" if $debug;
> +
> +    my $dst_volid = PVE::Storage::vdisk_alloc($storecfg, $storage_id, $vmid,
> +	$dst_format, undef, $src_size / 1024);
> +    my $dst_path = PVE::Storage::path($storecfg, $dst_volid);
> +
> +    warn "args:  $src_path, $vmid, $storage_id, $optional\n",
> +	"\$dst_volid: $dst_volid\n", if $debug;
> +
> +    # qemu-img convert does the hard job
> +    # we don't attempt to guess filetypes ourselves
> +    my $convert_command = ['qemu-img', 'convert', $src_path, '-p', '-n', '-O', $dst_format];
> +    if (PVE::Storage::volume_has_feature($storecfg, 'sparseinit', $dst_volid)) {
> +	push @$convert_command, "zeroinit:$dst_path";
> +    } else {
> +	push @$convert_command, $dst_path;
> +    }
> +
> +    my $create_drive = sub {
> +	my $vm_conf = PVE::QemuConfig->load_config($vmid);
> +	PVE::QemuConfig->check_lock($vm_conf);
> +
> +	if ($drive_name) {
> +	    if (exists $vm_conf->{$drive_name}) {

the exists is not needed here, is it? (we rarely use it in other places)

> +		# should never happen as setting $drive_name is not exposed to public interface
> +		die "cowardly refusing to overwrite existing entry: $drive_name";
> +	    } else {
> +		$vm_conf->{$drive_name} = PVE::QemuServer::print_drive($vmid,
> +		    { file => $dst_volid, size => $src_size });

this bypasses our pending/hotplugging mechanism, so it either needs to
be guarded with an is_running check, or needs to be modified to use the
hotplugging mechanism. I prefer the latter.

> +	    }
> +	} else {
> +	    PVE::QemuConfig->add_unused_volume($vm_conf, $dst_volid);

this is fine and works as intended :)

> +	}
> +	PVE::QemuConfig->write_config($vmid, $vm_conf);
> +    };
> +
> +    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]);
> +	PVE::QemuConfig->lock_config($vmid, $create_drive);
> +    };
> +
> +    my $err = $@;
> +    if ($err) {
> +	if (PVE::Storage::path($storecfg, $dst_volid)) {
> +	    PVE::Storage::vdisk_free($storecfg, $dst_volid);
> +	}

is the if really necessary here? we should already be sure that the
vdisk exists - we allocated it and got the path in $dst_path? we have no
guarantuee that it is still the same image we allocated anyway, as there
is no lock for this kind of interaction ;)

the vdisk_free should probably be inside an eval {} though - otherwise
an error in the cleanup masks the error which caused the cleanup, which
is confusing.

> +	die $err;
> +    }
> +}
> +
> +1;
> diff --git a/PVE/QemuServer/Makefile b/PVE/QemuServer/Makefile
> index 06617c5..f75f2e6 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 ImportDisk.pm ${DESTDIR}${PERLDIR}/PVE/QemuServer/ImportDisk.pm
> -- 
> 2.11.0
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel at pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel