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

[Fabian Grünbichler]

comments inline

On Tue, May 09, 2017 at 04:26:05PM +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 | 96 ++++++++++++++++++++++++++++++++++++++++++++
>  PVE/QemuServer/Makefile      |  1 +
>  2 files changed, 97 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..6d1ddbb
> --- /dev/null
> +++ b/PVE/QemuServer/ImportDisk.pm
> @@ -0,0 +1,96 @@
> +package PVE::QemuServer::ImportDisk;
> +
> +use strict;
> +use warnings;
> +use Data::Dumper;

I really really would like to avoid adding new code using Data::Dumper,
unless absolutely necessary.. in this case, it is only used to print
debug statements, which are not even exposed to anything public..

Also, a lot of the variables that are dumped are simple strings anyway..

> +
> +use PVE::Storage;
> +use PVE::Storage::Plugin;

see below, I'd like to get rid of this Plugin use.

> +use PVE::QemuServer;
> +use PVE::Tools 'run_command';
> +
> +# import an external disk image to an existing VM
> +# and create a drive entry unused[n] pointing to the created volume

this comment is wrong - if drive_name is passed, it will not create an
unused for the created volume (or at all, AFAICT).

> +# $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) = @_;

we usually pass such optional parameters in $params - not sure whether
this is a convention that we want to follow or not?

> +    my $dst = {};

is this hash really needed? it's passed to the helper function further
down, but that only uses two elements - volid and size. without it, a
lot of assignments are not needed anymore..

> +
> +    # get the needed size from  source disk
> +    my ($virtual_src_size, $src_format, $used, $parent) = PVE::Storage::Plugin::file_size_info($src_path);

please use the wrapper in Storage.pm (either file_siz_info or
volume_size_info) and drop the use for the ::Plugin part.

> +
> +    # get target format, target image's path, and whether it's possible to sparseinit
> +    my $storecfg = PVE::Storage::config();
> +    if ($optional && $optional->{format}) {
> +	$dst->{format} = PVE::QemuServer::choose_dst_disk_format($storecfg, $storage_id, undef, $optional->{format});

shouldn't this be called even if no explicit target format is requested?
we want to check for format compatibility before calling qemu-img in
both cases, same as for clone_disk...

> +	warn Dumper("format :", $dst->{format}) if $optional && $optional->{debug};
> +    }
> +    $dst->{volid} = PVE::Storage::vdisk_alloc($storecfg,
> +	$storage_id, $vmid, $dst->{format}, undef, $virtual_src_size / 1024);
> +    $dst->{path} = PVE::Storage::path($storecfg, $dst->{volid});
> +    $dst->{has_sparseinit} = PVE::Storage::volume_has_feature($storecfg, 'sparseinit', $dst->{volid});
> +    $dst->{size} = $virtual_src_size;

without dst, these two could be used as is down below.. has_sparseinit
is only used once anyway.

> +
> +    warn "args: ", Dumper($src_path, $vmid, $storage_id, $optional),
> +	"\$dst->{volid}: $dst->{volid}\n",
> +	"\$dst: ", Dumper($dst) if $optional && $optional->{debug};
> +
> +    # qemu-img convert does the hard job
> +    # we don't attempt to guest filetypes ourselves

s/guest/guess/

> +    my $convert_command = ['qemu-img', 'convert', $src_path, '-p', '-n'];
> +    if ($dst->{format}) {
> +	push @$convert_command, '-O', $dst->{format};
> +    }

see above..

> +    if ($dst->{has_sparseinit}) {

see above

> +	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, $vmid, $optional->{drive_name}, $dst);

imho this could be reorganized:
- drop dst
- make the two create subs from down below into one private sub
- inline the remaining content of __create_disk_entry using this new sub
- don't use "sub __foo", but "my $foo = sub" (the former is only needed
  for private subs which might need to be overloaded in derived modules)

> +    };
> +    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, $drive_name, $dst) = @_;
> +    if (PVE::QemuServer::vm_is_volid_owner($storecfg, $vmid, $dst->{volid})) {

unnecessary - we create the disk for this vmid, we know it's owned?

> +	my $vm_conf = PVE::QemuConfig->load_config($vmid);

wrong order, first lock, then load - otherwise this becomes racy.

> +
> +	my $create_active_drive = sub {
> +	    if (PVE::QemuServer::is_valid_drivename($drive_name)) {

shouldn't this check happen much earlier (i.e., before even allocating
an image?)

> +		$vm_conf->{$drive_name} = PVE::QemuServer::print_drive($vmid, { file => $dst->{volid}, size => $dst->{size} });

there was no check for an old value here, so now we potentially
unreferenced a still in use image without going through the normal
PENDING/unused mechanism. I suggest either checking and adding as unused
instead, or hotplugging altogether (which does add the old disk as
unused if one existed).

> +		PVE::QemuConfig->write_config($vmid, $vm_conf);
> +		return $drive_name;
> +	    }
> +	    return undef;
> +	};
> +
> +	my $create_unused_drive =  sub {
> +	    my $disk_key = PVE::QemuConfig->add_unused_volume($vm_conf, $dst->{volid});
> +	    PVE::QemuConfig->write_config($vmid, $vm_conf);
> +	    return $disk_key;
> +	};
> +
> +	my $create_entry = $drive_name ? $create_active_drive : $create_unused_drive;
> +	return PVE::QemuConfig->lock_config($vmid, $create_entry);
> +    }
> +}
> +
> +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