[pve-devel] [PATCH qemu-server 2/2] fix #2395: refactor qemu_img_convert to accept files as source

Thomas Lamprecht t.lamprecht at proxmox.com
Thu Oct 17 09:35:51 CEST 2019


On 10/16/19 11:27 AM, Dominik Csapak wrote:
> and use it also for efidisk creation and importdisk
> this way we correctly handle zfs-over-iscsi options for those cases
> 
> also write tests for it
> 

one minor nit about a error message below, with that:

Reviewed-by: Thomas Lamprecht <t.lamprecht at proxmox.com>

> Signed-off-by: Dominik Csapak <d.csapak at proxmox.com>
> ---
> makes more sense when viewed with -w, but i think comparing the
> complete source of qemu_img_convert before and after this patch is easiest
> 
>  PVE/QemuServer.pm                  | 112 +++++++++++++++--------------
>  PVE/QemuServer/ImportDisk.pm       |  11 +--
>  test/run_qemu_img_convert_tests.pl |  25 +++++++
>  test/test.vmdk                     |   0
>  4 files changed, 87 insertions(+), 61 deletions(-)
>  create mode 100644 test/test.vmdk
> 
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index 8dda594..2388eab 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -6884,66 +6884,77 @@ sub qemu_img_convert {
>      my ($src_storeid, $src_volname) = PVE::Storage::parse_volume_id($src_volid, 1);
>      my ($dst_storeid, $dst_volname) = PVE::Storage::parse_volume_id($dst_volid, 1);
>  
> -    if ($src_storeid && $dst_storeid) {
> +    die "invalid destination for qemu-img convert\n" if !$dst_storeid;
>  
> -	PVE::Storage::activate_volumes($storecfg, [$src_volid], $snapname);
> +    my $cachemode;
> +    my $src_path;
> +    my $src_is_iscsi = 0;
> +    my $src_format = 'raw';
>  
> +    if ($src_storeid) {
> +	PVE::Storage::activate_volumes($storecfg, [$src_volid], $snapname);
>  	my $src_scfg = PVE::Storage::storage_config($storecfg, $src_storeid);
> -	my $dst_scfg = PVE::Storage::storage_config($storecfg, $dst_storeid);
> +	$src_format = qemu_img_format($src_scfg, $src_volname);
> +	$src_path = PVE::Storage::path($storecfg, $src_volid, $snapname);
> +	$src_is_iscsi = ($src_path =~ m|^iscsi://|);
> +	$cachemode = 'none' if $src_scfg->{type} eq 'zfspool';
> +    } elsif (-f $src_volid) {
> +	$src_path = $src_volid;
> +	if ($src_path =~ m/\.($QEMU_FORMAT_RE)$/) {
> +	    $src_format = $1;
> +	}
> +    }
>  
> -	my $src_format = qemu_img_format($src_scfg, $src_volname);
> -	my $dst_format = qemu_img_format($dst_scfg, $dst_volname);
> +    die "invalid source for qemu-img convert\n" if !$src_path;

I'd output the $src_volid so we/users can know why this triggered.
Something like the following, in nice: 
"invalid source for qemu-img convert, '$src_volid' neither valid volume id nor exist as file!"

>  
> -	my $src_path = PVE::Storage::path($storecfg, $src_volid, $snapname);
> -	my $dst_path = PVE::Storage::path($storecfg, $dst_volid);
> +    my $dst_scfg = PVE::Storage::storage_config($storecfg, $dst_storeid);
> +    my $dst_format = qemu_img_format($dst_scfg, $dst_volname);
> +    my $dst_path = PVE::Storage::path($storecfg, $dst_volid);
> +    my $dst_is_iscsi = ($dst_path =~ m|^iscsi://|);
>  
> -	my $src_is_iscsi = ($src_path =~ m|^iscsi://|);
> -	my $dst_is_iscsi = ($dst_path =~ m|^iscsi://|);
> +    my $cmd = [];
> +    push @$cmd, '/usr/bin/qemu-img', 'convert', '-p', '-n';
> +    push @$cmd, '-l', "snapshot.name=$snapname" if($snapname && $src_format eq "qcow2");
> +    push @$cmd, '-t', 'none' if $dst_scfg->{type} eq 'zfspool';
> +    push @$cmd, '-T', $cachemode if defined($cachemode);
> +
> +    if ($src_is_iscsi) {
> +	push @$cmd, '--image-opts';
> +	$src_path = convert_iscsi_path($src_path);
> +    } else {
> +	push @$cmd, '-f', $src_format;
> +    }
>  
> -	my $cmd = [];
> -	push @$cmd, '/usr/bin/qemu-img', 'convert', '-p', '-n';
> -	push @$cmd, '-l', "snapshot.name=$snapname" if($snapname && $src_format eq "qcow2");
> -	push @$cmd, '-t', 'none' if $dst_scfg->{type} eq 'zfspool';
> -	push @$cmd, '-T', 'none' if $src_scfg->{type} eq 'zfspool';
> +    if ($dst_is_iscsi) {
> +	push @$cmd, '--target-image-opts';
> +	$dst_path = convert_iscsi_path($dst_path);
> +    } else {
> +	push @$cmd, '-O', $dst_format;
> +    }
>  
> -	if ($src_is_iscsi) {
> -	    push @$cmd, '--image-opts';
> -	    $src_path = convert_iscsi_path($src_path);
> -	} else {
> -	    push @$cmd, '-f', $src_format;
> -	}
> +    push @$cmd, $src_path;
>  
> -	if ($dst_is_iscsi) {
> -	    push @$cmd, '--target-image-opts';
> -	    $dst_path = convert_iscsi_path($dst_path);
> -	} else {
> -	    push @$cmd, '-O', $dst_format;
> -	}
> +    if (!$dst_is_iscsi && $is_zero_initialized) {
> +	push @$cmd, "zeroinit:$dst_path";
> +    } else {
> +	push @$cmd, $dst_path;
> +    }
>  
> -	push @$cmd, $src_path;
> +    my $parser = sub {
> +	my $line = shift;
> +	if($line =~ m/\((\S+)\/100\%\)/){
> +	    my $percent = $1;
> +	    my $transferred = int($size * $percent / 100);
> +	    my $remaining = $size - $transferred;
>  
> -	if (!$dst_is_iscsi && $is_zero_initialized) {
> -	    push @$cmd, "zeroinit:$dst_path";
> -	} else {
> -	    push @$cmd, $dst_path;
> +	    print "transferred: $transferred bytes remaining: $remaining bytes total: $size bytes progression: $percent %\n";
>  	}
>  
> -	my $parser = sub {
> -	    my $line = shift;
> -	    if($line =~ m/\((\S+)\/100\%\)/){
> -		my $percent = $1;
> -		my $transferred = int($size * $percent / 100);
> -		my $remaining = $size - $transferred;
> -
> -		print "transferred: $transferred bytes remaining: $remaining bytes total: $size bytes progression: $percent %\n";
> -	    }
> -
> -	};
> +    };
>  
> -	eval  { run_command($cmd, timeout => undef, outfunc => $parser); };
> -	my $err = $@;
> -	die "copy failed: $err" if $err;
> -    }
> +    eval  { run_command($cmd, timeout => undef, outfunc => $parser); };
> +    my $err = $@;
> +    die "copy failed: $err" if $err;
>  }
>  
>  sub qemu_img_format {
> @@ -7269,15 +7280,12 @@ sub create_efidisk($$$$$) {
>      my (undef, $ovmf_vars) = get_ovmf_files($arch);
>      die "EFI vars default image not found\n" if ! -f $ovmf_vars;
>  
> -    my $vars_size = PVE::Tools::convert_size(-s $ovmf_vars, 'b' => 'kb');
> +    my $vars_size_b = -s $ovmf_vars;
> +    my $vars_size = PVE::Tools::convert_size($vars_size_b, 'b' => 'kb');
>      my $volid = PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, $fmt, undef, $vars_size);
>      PVE::Storage::activate_volumes($storecfg, [$volid]);
>  
> -    my $path = PVE::Storage::path($storecfg, $volid);
> -    eval {
> -	run_command(['/usr/bin/qemu-img', 'convert', '-n', '-f', 'raw', '-O', $fmt, $ovmf_vars, $path]);
> -    };
> -    die "Copying EFI vars image failed: $@" if $@;
> +    qemu_img_convert($ovmf_vars, $volid, $vars_size_b, undef, 0);
>  
>      return ($volid, $vars_size);
>  }
> diff --git a/PVE/QemuServer/ImportDisk.pm b/PVE/QemuServer/ImportDisk.pm
> index db7db63..5d391e6 100755
> --- a/PVE/QemuServer/ImportDisk.pm
> +++ b/PVE/QemuServer/ImportDisk.pm
> @@ -37,14 +37,7 @@ sub do_import {
>      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 $zeroinit = PVE::Storage::volume_has_feature($storecfg, 'sparseinit', $dst_volid);
>  
>      my $create_drive = sub {
>  	my $vm_conf = PVE::QemuConfig->load_config($vmid);
> @@ -88,7 +81,7 @@ sub do_import {
>  	    local $SIG{HUP} =
>  	    local $SIG{PIPE} = sub { die "interrupted by signal\n"; };
>  	PVE::Storage::activate_volumes($storecfg, [$dst_volid]);
> -	run_command($convert_command);
> +	PVE::QemuServer::qemu_img_convert($src_path, $dst_volid, $src_size, undef, $zeroinit);
>  	PVE::Storage::deactivate_volumes($storecfg, [$dst_volid]);
>  	PVE::QemuConfig->lock_config($vmid, $create_drive);
>      };
> diff --git a/test/run_qemu_img_convert_tests.pl b/test/run_qemu_img_convert_tests.pl
> index fafb58a..5ad037f 100755
> --- a/test/run_qemu_img_convert_tests.pl
> +++ b/test/run_qemu_img_convert_tests.pl
> @@ -129,6 +129,31 @@ my $tests = {
>  	"local-lvm:vm-$vmid-disk-0", "not-existing:$vmid/vm-$vmid-disk-0.raw", 1024*10, undef, 1,
>  	"storage 'not-existing' does not exists\n",
>      ],
> +    vmdkfile => [
> +	"./test.vmdk", "local:$vmid/vm-$vmid-disk-0.raw", 1024*10, undef, 0, [
> +	    "/usr/bin/qemu-img", "convert", "-p", "-n", "-f", "vmdk", "-O", "raw",
> +	    "./test.vmdk",
> +	    "/var/lib/vz/images/$vmid/vm-$vmid-disk-0.raw",
> +	]
> +    ],
> +    notexistingfile => [
> +	"/foo/bar", "local:$vmid/vm-$vmid-disk-0.raw", 1024*10, undef, 0,
> +	"invalid source for qemu-img convert\n",
> +    ],
> +    efidisk => [
> +	"/usr/share/kvm/OVMF_VARS-pure-efi.fd", "local:$vmid/vm-$vmid-disk-0.raw", 1024*10, undef, 0, [
> +	    "/usr/bin/qemu-img", "convert", "-p", "-n", "-f", "raw", "-O", "raw",
> +	    "/usr/share/kvm/OVMF_VARS-pure-efi.fd",
> +	    "/var/lib/vz/images/$vmid/vm-$vmid-disk-0.raw",
> +	]
> +    ],
> +    efi2zos => [
> +	"/usr/share/kvm/OVMF_VARS-pure-efi.fd", "zfs-over-iscsi:vm-$vmid-disk-0", 1024*10, undef, 0, [
> +	    "/usr/bin/qemu-img", "convert", "-p", "-n", "-f", "raw", "--target-image-opts",
> +	    "/usr/share/kvm/OVMF_VARS-pure-efi.fd",
> +	    "file.driver=iscsi,file.transport=tcp,file.initiator-name=foobar,file.portal=127.0.0.1,file.target=iqn.2019-10.org.test:foobar,file.lun=1,driver=raw",
> +	]
> +    ],
>  };
>  
>  my $command;
> diff --git a/test/test.vmdk b/test/test.vmdk
> new file mode 100644
> index 0000000..e69de29
> 





More information about the pve-devel mailing list