[pve-devel] [PATCH qemu-server 1/2] Add DiskImport module to import external disk images into a VM
Alexandre DERUMIER
aderumier at odiso.com
Tue Apr 25 19:04:31 CEST 2017
>>> + 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};
>>> + }
I need to test, but I'm pretty sure that we don't need zeroinit for qemu-img.
(as qemu-img skip zero, and don't write them on target).
Currently we don't use it for qemu-img convert in offline vm cloning.
It's only needed for online qemu drive-mirror.
----- Mail original -----
De: "Thomas Lamprecht" <t.lamprecht at proxmox.com>
À: "pve-devel" <pve-devel at pve.proxmox.com>
Envoyé: Mardi 25 Avril 2017 15:18:15
Objet: Re: [pve-devel] [PATCH qemu-server 1/2] Add DiskImport module to import external disk images into a VM
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
_______________________________________________
pve-devel mailing list
pve-devel at pve.proxmox.com
http://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
More information about the pve-devel
mailing list