[pve-devel] [PATCH-SERIES v12 qemu-server/manager] API for disk import and OVF

[pve-devel] [PATCH-SERIES v12 qemu-server/manager] API for disk import and OVF

[Fabian Ebner]

Extend qm importdisk/importovf functionality to the API.

Changes from v11 (after an off-list discussion with Fabian G.):
    * Avoid confusing $dest->{conf} parameter for clone_disk().
    * Require relevant parameters to be set explicitly for EFI/TPM.
    * Base calculation of EFI vars size on passed-in parameters.

Changes from v10:
    * Add fix for device unplug issue (patch #1).
    * Add fixes related to calling create_disks() (patches #2 #3).
    * Refactor clone_disk() in preparation to re-use it for import
      (patches #4 #5 #6).
    * Add patch to print the newly allocated drive (patch #14).
    * Switch to using clone_disk for PVE-managed volumes and check for
      VM.Clone in the permission check if there is an owner ID.
    * Require <storeid>:0 syntax when using import-from. Allowing
      other values than 0 for the size would be confusing, because
      with import-from that size is never used (the size of the source
      image is).
    * Avoid making all foreach_volume iterators parse with the
      extended schema. Instead, provide a custom iterator for the
      places where it's actually required.

Still missing GUI integration for import from ovf, but that will be it's
own series.

Older discussion:
https://lists.proxmox.com/pipermail/pve-devel/2022-January/051379.html


qemu-server:

Dominic Jäger (1):
  api: support VM disk import

Fabian Ebner (15):
  device unplug: verify that unplugging scsi disk completed
  api: create disks: always activate/update size when attaching existing
    volume
  api: update: pass correct config when creating disks
  clone disk: remove check for min QEMU version 2.7
  clone disk: group source and target parameters
  clone disk: pass in efi vars size rather than config
  clone disk: allow cloning from an unused or unreferenced disk
  efivars size: allow overriding efidisk parameter
  schema: add pve-volume-id-or-absolute-path
  parse ovf: untaint path when calling file_size_info
  api: add endpoint for parsing .ovf files
  image convert: allow block device as source
  api: factor out check/cleanup for drive params
  schema: drive: use separate schema when disk allocation is possible
  api: update vm: print drive string for newly allocated/imported drives

 PVE/API2/Qemu.pm             | 381 +++++++++++++++++++++++++++--------
 PVE/API2/Qemu/Makefile       |   2 +-
 PVE/API2/Qemu/OVF.pm         |  55 +++++
 PVE/QemuServer.pm            | 106 +++++++---
 PVE/QemuServer/Drive.pm      |  94 ++++++---
 PVE/QemuServer/ImportDisk.pm |   2 +-
 PVE/QemuServer/OVF.pm        |   9 +-
 7 files changed, 508 insertions(+), 141 deletions(-)
 create mode 100644 PVE/API2/Qemu/OVF.pm


manager:

Fabian Ebner (1):
  api: nodes: add readovf endpoint

 PVE/API2/Nodes.pm | 7 +++++++
 1 file changed, 7 insertions(+)

-- 
2.30.2

[pve-devel] [PATCH v12 qemu-server 01/16] device unplug: verify that unplugging scsi disk completed

[Fabian Ebner]

Avoids the error
  adding drive failed: Duplicate ID 'drive-scsi1' for drive
that could happen when switching over to a new disk (e.g. via qm set),
if unplugging wasn't fast enough.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 PVE/QemuServer.pm | 1 +
 1 file changed, 1 insertion(+)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 42f0fbd..b7e6a8e 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -4249,6 +4249,7 @@ sub vm_deviceunplug {
 	my $device = parse_drive($deviceid, $conf->{$deviceid});
 
 	qemu_devicedel($vmid, $deviceid);
+	qemu_devicedelverify($vmid, $deviceid);
 	qemu_drivedel($vmid, $deviceid);
 	qemu_deletescsihw($conf, $vmid, $deviceid);
 
-- 
2.30.2

[pve-devel] [PATCH v12 qemu-server 02/16] api: create disks: always activate/update size when attaching existing volume

[Fabian Ebner]

For creation, activation and size update never triggered, because the
passed in $conf is essentially the same as the creation $settings, so
the disk was always detected to be the same as the "existing" one. But
actually, all disks are new, so it makes sense to do it.

For update, activation and size update nearly always triggered,
because only the pending changes are passed in as $conf. The case
where it didn't trigger is when the same pending change was made twice
(there are cases where hotplug isn't done, but makes it even more
unlikely).

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 PVE/API2/Qemu.pm | 21 ++++-----------------
 1 file changed, 4 insertions(+), 17 deletions(-)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 9be1caf..02b26d2 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -213,26 +213,13 @@ my $create_disks = sub {
 	    delete $disk->{format}; # no longer needed
 	    $res->{$ds} = PVE::QemuServer::print_drive($disk);
 	} else {
-
 	    PVE::Storage::check_volume_access($rpcenv, $authuser, $storecfg, $vmid, $volid);
 
-	    my $volid_is_new = 1;
-
-	    if ($conf->{$ds}) {
-		my $olddrive = PVE::QemuServer::parse_drive($ds, $conf->{$ds});
-		$volid_is_new = undef if $olddrive->{file} && $olddrive->{file} eq $volid;
-	    }
-
-	    if ($volid_is_new) {
+	    PVE::Storage::activate_volumes($storecfg, [ $volid ]) if $storeid;
 
-		PVE::Storage::activate_volumes($storecfg, [ $volid ]) if $storeid;
-
-		my $size = PVE::Storage::volume_size_info($storecfg, $volid);
-
-		die "volume $volid does not exist\n" if !$size;
-
-		$disk->{size} = $size;
-	    }
+	    my $size = PVE::Storage::volume_size_info($storecfg, $volid);
+	    die "volume $volid does not exist\n" if !$size;
+	    $disk->{size} = $size;
 
 	    $res->{$ds} = PVE::QemuServer::print_drive($disk);
 	}
-- 
2.30.2

[pve-devel] [PATCH v12 qemu-server 03/16] api: update: pass correct config when creating disks

[Fabian Ebner]

While the new options should be written to the pending config, the
decisions (currently only one) in create_disks needs to be made for
the current config.

Seems to fix EFI disk creation, but actually, it's only
future-proofing, because, currently, the same OVMF_VARS file is
used independently of $smm.

The correct config is also needed to determine the correct size for
the EFI disk for the upcoming import-from feature.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 PVE/API2/Qemu.pm | 32 ++++++++++++++++++++++++--------
 1 file changed, 24 insertions(+), 8 deletions(-)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 02b26d2..c6587ef 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -237,12 +237,7 @@ my $create_disks = sub {
 	die $err;
     }
 
-    # modify vm config if everything went well
-    foreach my $ds (keys %$res) {
-	$conf->{$ds} = $res->{$ds};
-    }
-
-    return $vollist;
+    return ($vollist, $res);
 };
 
 my $check_cpu_model_access = sub {
@@ -712,7 +707,18 @@ __PACKAGE__->register_method({
 
 		my $vollist = [];
 		eval {
-		    $vollist = &$create_disks($rpcenv, $authuser, $conf, $arch, $storecfg, $vmid, $pool, $param, $storage);
+		    ($vollist, my $created_opts) = $create_disks->(
+			$rpcenv,
+			$authuser,
+			$conf,
+			$arch,
+			$storecfg,
+			$vmid,
+			$pool,
+			$param,
+			$storage,
+		    );
+		    $conf->{$_} = $created_opts->{$_} for keys $created_opts->%*;
 
 		    if (!$conf->{boot}) {
 			my $devs = PVE::QemuServer::get_default_bootdevices($conf);
@@ -1364,7 +1370,17 @@ my $update_vm_api  = sub {
 		    PVE::QemuServer::vmconfig_register_unused_drive($storecfg, $vmid, $conf, PVE::QemuServer::parse_drive($opt, $conf->{pending}->{$opt}))
 			if defined($conf->{pending}->{$opt});
 
-		    &$create_disks($rpcenv, $authuser, $conf->{pending}, $arch, $storecfg, $vmid, undef, {$opt => $param->{$opt}});
+		    my (undef, $created_opts) = $create_disks->(
+			$rpcenv,
+			$authuser,
+			$conf,
+			$arch,
+			$storecfg,
+			$vmid,
+			undef,
+			{$opt => $param->{$opt}},
+		    );
+		    $conf->{pending}->{$_} = $created_opts->{$_} for keys $created_opts->%*;
 
 		    # default legacy boot order implies all cdroms anyway
 		    if (@bootorder) {
-- 
2.30.2

[pve-devel] [PATCH v12 qemu-server 04/16] clone disk: remove check for min QEMU version 2.7

[Fabian Ebner]

Upgrading a cluster node entails re-starting or migrating VMs and even
PVE 6.0 already had QEMU 4.0.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 PVE/QemuServer.pm | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index b7e6a8e..c0fca49 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -7642,15 +7642,8 @@ sub clone_disk {
 		qemu_img_convert($drive->{file}, $newvolid, $size, $snapname, $sparseinit);
 	    }
 	} else {
-
 	    die "cannot move TPM state while VM is running\n" if $drivename eq 'tpmstate0';
 
-	    my $kvmver = get_running_qemu_version ($vmid);
-	    if (!min_version($kvmver, 2, 7)) {
-		die "drive-mirror with iothread requires qemu version 2.7 or higher\n"
-		    if $drive->{iothread};
-	    }
-
 	    qemu_drive_mirror($vmid, $drivename, $newvolid, $newvmid, $sparseinit, $jobs,
 	        $completion, $qga, $bwlimit);
 	}
-- 
2.30.2

[pve-devel] [PATCH v12 qemu-server 05/16] clone disk: group source and target parameters

[Fabian Ebner]

to make the interface more digestible.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 PVE/API2/Qemu.pm  | 52 +++++++++++++++++++++++++++++++----------------
 PVE/QemuServer.pm |  9 ++++++--
 2 files changed, 41 insertions(+), 20 deletions(-)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index c6587ef..14cac5b 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -3218,23 +3218,31 @@ __PACKAGE__->register_method({
 		    push @$storage_list, $storage if defined($storage);
 		    my $clonelimit = PVE::Storage::get_bandwidth_limit('clone', $storage_list, $bwlimit);
 
+		    my $source_info = {
+			vmid => $vmid,
+			running => $running,
+			drivename => $opt,
+			drive => $drive,
+			snapname => $snapname,
+		    };
+
+		    my $dest_info = {
+			vmid => $newid,
+			conf => $oldconf, # because it's a clone
+			storage => $storage,
+			format => $format,
+		    };
+
 		    my $newdrive = PVE::QemuServer::clone_disk(
 			$storecfg,
-			$vmid,
-			$running,
-			$opt,
-			$drive,
-			$snapname,
-			$newid,
-			$storage,
-			$format,
+			$source_info,
+			$dest_info,
 			$fullclone->{$opt},
 			$newvollist,
 			$jobs,
 			$completion,
 			$oldconf->{agent},
 			$clonelimit,
-			$oldconf
 		    );
 
 		    $newconf->{$opt} = PVE::QemuServer::print_drive($newdrive);
@@ -3469,23 +3477,31 @@ __PACKAGE__->register_method({
 		    $bwlimit
 		);
 
+		my $source_info = {
+		    vmid => $vmid,
+		    running => $running,
+		    drivename => $disk,
+		    drive => $drive,
+		    snapname => undef,
+		};
+
+		my $dest_info = {
+		    vmid => $vmid,
+		    conf => $conf,
+		    storage => $storeid,
+		    format => $format,
+		};
+
 		my $newdrive = PVE::QemuServer::clone_disk(
 		    $storecfg,
-		    $vmid,
-		    $running,
-		    $disk,
-		    $drive,
-		    undef,
-		    $vmid,
-		    $storeid,
-		    $format,
+		    $source_info,
+		    $dest_info,
 		    1,
 		    $newvollist,
 		    undef,
 		    undef,
 		    undef,
 		    $movelimit,
-		    $conf,
 		);
 		$conf->{$disk} = PVE::QemuServer::print_drive($newdrive);
 
diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index c0fca49..56437c5 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -7571,8 +7571,13 @@ sub qemu_blockjobs_cancel {
 }
 
 sub clone_disk {
-    my ($storecfg, $vmid, $running, $drivename, $drive, $snapname,
-	$newvmid, $storage, $format, $full, $newvollist, $jobs, $completion, $qga, $bwlimit, $conf) = @_;
+    my ($storecfg, $source, $dest, $full, $newvollist, $jobs, $completion, $qga, $bwlimit) = @_;
+
+    my ($vmid, $running) = $source->@{qw(vmid running)};
+    my ($drivename, $drive, $snapname) = $source->@{qw(drivename drive snapname)};
+
+    my ($newvmid, $conf) = $dest->@{qw(vmid conf)};
+    my ($storage, $format) = $dest->@{qw(storage format)};
 
     my $newvolid;
 
-- 
2.30.2

[pve-devel] [PATCH v12 qemu-server 06/16] clone disk: pass in efi vars size rather than config

[Fabian Ebner]

It's confusing that the config associated to the destination is
actually a reference to the source config for both existing callers.

Also, disk import will need to base the calculation on the passed-in
drive parameters and not just the current config, so this change is in
preparation for that too.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

New in v12.

 PVE/API2/Qemu.pm  | 8 ++++++--
 PVE/QemuServer.pm | 4 ++--
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 14cac5b..9b8eb88 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -3228,11 +3228,13 @@ __PACKAGE__->register_method({
 
 		    my $dest_info = {
 			vmid => $newid,
-			conf => $oldconf, # because it's a clone
 			storage => $storage,
 			format => $format,
 		    };
 
+		    $dest_info->{efisize} = PVE::QemuServer::get_efivars_size($oldconf)
+			if $opt eq 'efidisk0';
+
 		    my $newdrive = PVE::QemuServer::clone_disk(
 			$storecfg,
 			$source_info,
@@ -3487,11 +3489,13 @@ __PACKAGE__->register_method({
 
 		my $dest_info = {
 		    vmid => $vmid,
-		    conf => $conf,
 		    storage => $storeid,
 		    format => $format,
 		};
 
+		$dest_info->{efisize} = PVE::QemuServer::get_efivars_size($conf)
+		    if $disk eq 'efidisk0';
+
 		my $newdrive = PVE::QemuServer::clone_disk(
 		    $storecfg,
 		    $source_info,
diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 56437c5..ed06239 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -7576,7 +7576,7 @@ sub clone_disk {
     my ($vmid, $running) = $source->@{qw(vmid running)};
     my ($drivename, $drive, $snapname) = $source->@{qw(drivename drive snapname)};
 
-    my ($newvmid, $conf) = $dest->@{qw(vmid conf)};
+    my ($newvmid, $efisize) = $dest->@{qw(vmid efisize)};
     my ($storage, $format) = $dest->@{qw(storage format)};
 
     my $newvolid;
@@ -7604,7 +7604,7 @@ sub clone_disk {
 	    $snapname = undef;
 	    $size = PVE::QemuServer::Cloudinit::CLOUDINIT_DISK_SIZE;
 	} elsif ($drivename eq 'efidisk0') {
-	    $size = get_efivars_size($conf);
+	    $size = $efisize or die "internal error - need to specify EFI disk size\n";
 	} elsif ($drivename eq 'tpmstate0') {
 	    $dst_format = 'raw';
 	    $size = PVE::QemuServer::Drive::TPMSTATE_DISK_SIZE;
-- 
2.30.2

[pve-devel] [PATCH v12 qemu-server 07/16] clone disk: allow cloning from an unused or unreferenced disk

[Fabian Ebner]

and also when source and target drivename are different. In those
cases, it is done via qemu-img convert/dd.

In preparation to allow import from existing PVE-managed disks.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 PVE/API2/Qemu.pm  |  2 ++
 PVE/QemuServer.pm | 29 +++++++++++++++++++----------
 2 files changed, 21 insertions(+), 10 deletions(-)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 9b8eb88..3b86034 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -3228,6 +3228,7 @@ __PACKAGE__->register_method({
 
 		    my $dest_info = {
 			vmid => $newid,
+			drivename => $opt,
 			storage => $storage,
 			format => $format,
 		    };
@@ -3489,6 +3490,7 @@ __PACKAGE__->register_method({
 
 		my $dest_info = {
 		    vmid => $vmid,
+		    drivename => $disk,
 		    storage => $storeid,
 		    format => $format,
 		};
diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index ed06239..b98ed3d 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -7574,15 +7574,25 @@ sub clone_disk {
     my ($storecfg, $source, $dest, $full, $newvollist, $jobs, $completion, $qga, $bwlimit) = @_;
 
     my ($vmid, $running) = $source->@{qw(vmid running)};
-    my ($drivename, $drive, $snapname) = $source->@{qw(drivename drive snapname)};
+    my ($src_drivename, $drive, $snapname) = $source->@{qw(drivename drive snapname)};
 
-    my ($newvmid, $efisize) = $dest->@{qw(vmid efisize)};
+    my ($newvmid, $dst_drivename, $efisize) = $dest->@{qw(vmid drivename efisize)};
     my ($storage, $format) = $dest->@{qw(storage format)};
 
+    if ($src_drivename && $dst_drivename && $src_drivename ne $dst_drivename) {
+	die "cloning from/to EFI disk requires EFI disk\n"
+	    if $src_drivename eq 'efidisk0' || $dst_drivename eq 'efidisk0';
+	die "cloning from/to TPM state requires TPM state\n"
+	    if $src_drivename eq 'tpmstate0' || $dst_drivename eq 'tpmstate0';
+    }
+
     my $newvolid;
 
+    print "create " . ($full ? 'full' : 'linked') . " clone of drive ";
+    print "$src_drivename " if $src_drivename;
+    print "($drive->{file})\n";
+
     if (!$full) {
-	print "create linked clone of drive $drivename ($drive->{file})\n";
 	$newvolid = PVE::Storage::vdisk_clone($storecfg,  $drive->{file}, $newvmid, $snapname);
 	push @$newvollist, $newvolid;
     } else {
@@ -7592,7 +7602,6 @@ sub clone_disk {
 
 	my $dst_format = resolve_dst_disk_format($storecfg, $storeid, $volname, $format);
 
-	print "create full clone of drive $drivename ($drive->{file})\n";
 	my $name = undef;
 	my $size = undef;
 	if (drive_is_cloudinit($drive)) {
@@ -7603,9 +7612,9 @@ sub clone_disk {
 	    }
 	    $snapname = undef;
 	    $size = PVE::QemuServer::Cloudinit::CLOUDINIT_DISK_SIZE;
-	} elsif ($drivename eq 'efidisk0') {
+	} elsif ($dst_drivename eq 'efidisk0') {
 	    $size = $efisize or die "internal error - need to specify EFI disk size\n";
-	} elsif ($drivename eq 'tpmstate0') {
+	} elsif ($dst_drivename eq 'tpmstate0') {
 	    $dst_format = 'raw';
 	    $size = PVE::QemuServer::Drive::TPMSTATE_DISK_SIZE;
 	} else {
@@ -7629,9 +7638,9 @@ sub clone_disk {
 	}
 
 	my $sparseinit = PVE::Storage::volume_has_feature($storecfg, 'sparseinit', $newvolid);
-	if (!$running || $snapname) {
+	if (!$running || !$src_drivename || $snapname) {
 	    # TODO: handle bwlimits
-	    if ($drivename eq 'efidisk0') {
+	    if ($dst_drivename eq 'efidisk0') {
 		# the relevant data on the efidisk may be smaller than the source
 		# e.g. on RBD/ZFS, so we use dd to copy only the amount
 		# that is given by the OVMF_VARS.fd
@@ -7647,9 +7656,9 @@ sub clone_disk {
 		qemu_img_convert($drive->{file}, $newvolid, $size, $snapname, $sparseinit);
 	    }
 	} else {
-	    die "cannot move TPM state while VM is running\n" if $drivename eq 'tpmstate0';
+	    die "cannot move TPM state while VM is running\n" if $src_drivename eq 'tpmstate0';
 
-	    qemu_drive_mirror($vmid, $drivename, $newvolid, $newvmid, $sparseinit, $jobs,
+	    qemu_drive_mirror($vmid, $src_drivename, $newvolid, $newvmid, $sparseinit, $jobs,
 	        $completion, $qga, $bwlimit);
 	}
     }
-- 
2.30.2

Re: [pve-devel] [PATCH v12 qemu-server 07/16] clone disk: allow cloning from an unused or unreferenced disk

[Fabian Grünbichler]

On March 9, 2022 11:09 am, Fabian Ebner wrote:
> and also when source and target drivename are different. In those
> cases, it is done via qemu-img convert/dd.
> 
> In preparation to allow import from existing PVE-managed disks.
> 
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
>  PVE/API2/Qemu.pm  |  2 ++
>  PVE/QemuServer.pm | 29 +++++++++++++++++++----------
>  2 files changed, 21 insertions(+), 10 deletions(-)
> 
> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
> index 9b8eb88..3b86034 100644
> --- a/PVE/API2/Qemu.pm
> +++ b/PVE/API2/Qemu.pm
> @@ -3228,6 +3228,7 @@ __PACKAGE__->register_method({
>  
>  		    my $dest_info = {
>  			vmid => $newid,
> +			drivename => $opt,
>  			storage => $storage,
>  			format => $format,
>  		    };
> @@ -3489,6 +3490,7 @@ __PACKAGE__->register_method({
>  
>  		my $dest_info = {
>  		    vmid => $vmid,
> +		    drivename => $disk,
>  		    storage => $storeid,
>  		    format => $format,
>  		};
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index ed06239..b98ed3d 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -7574,15 +7574,25 @@ sub clone_disk {
>      my ($storecfg, $source, $dest, $full, $newvollist, $jobs, $completion, $qga, $bwlimit) = @_;
>  
>      my ($vmid, $running) = $source->@{qw(vmid running)};
> -    my ($drivename, $drive, $snapname) = $source->@{qw(drivename drive snapname)};
> +    my ($src_drivename, $drive, $snapname) = $source->@{qw(drivename drive snapname)};
>  
> -    my ($newvmid, $efisize) = $dest->@{qw(vmid efisize)};
> +    my ($newvmid, $dst_drivename, $efisize) = $dest->@{qw(vmid drivename efisize)};
>      my ($storage, $format) = $dest->@{qw(storage format)};
>  
> +    if ($src_drivename && $dst_drivename && $src_drivename ne $dst_drivename) {
> +	die "cloning from/to EFI disk requires EFI disk\n"
> +	    if $src_drivename eq 'efidisk0' || $dst_drivename eq 'efidisk0';
> +	die "cloning from/to TPM state requires TPM state\n"
> +	    if $src_drivename eq 'tpmstate0' || $dst_drivename eq 'tpmstate0';

potential follow-up: should we check `$running && !$snapname` here (continued below)

> +    }
> +
>      my $newvolid;
>  
> +    print "create " . ($full ? 'full' : 'linked') . " clone of drive ";
> +    print "$src_drivename " if $src_drivename;
> +    print "($drive->{file})\n";
> +
>      if (!$full) {
> -	print "create linked clone of drive $drivename ($drive->{file})\n";
>  	$newvolid = PVE::Storage::vdisk_clone($storecfg,  $drive->{file}, $newvmid, $snapname);
>  	push @$newvollist, $newvolid;
>      } else {
> @@ -7592,7 +7602,6 @@ sub clone_disk {
>  
>  	my $dst_format = resolve_dst_disk_format($storecfg, $storeid, $volname, $format);
>  
> -	print "create full clone of drive $drivename ($drive->{file})\n";
>  	my $name = undef;
>  	my $size = undef;
>  	if (drive_is_cloudinit($drive)) {
> @@ -7603,9 +7612,9 @@ sub clone_disk {
>  	    }
>  	    $snapname = undef;
>  	    $size = PVE::QemuServer::Cloudinit::CLOUDINIT_DISK_SIZE;
> -	} elsif ($drivename eq 'efidisk0') {
> +	} elsif ($dst_drivename eq 'efidisk0') {
>  	    $size = $efisize or die "internal error - need to specify EFI disk size\n";
> -	} elsif ($drivename eq 'tpmstate0') {
> +	} elsif ($dst_drivename eq 'tpmstate0') {
>  	    $dst_format = 'raw';
>  	    $size = PVE::QemuServer::Drive::TPMSTATE_DISK_SIZE;
>  	} else {
> @@ -7629,9 +7638,9 @@ sub clone_disk {
>  	}
>  
>  	my $sparseinit = PVE::Storage::volume_has_feature($storecfg, 'sparseinit', $newvolid);
> -	if (!$running || $snapname) {
> +	if (!$running || !$src_drivename || $snapname) {
>  	    # TODO: handle bwlimits
> -	    if ($drivename eq 'efidisk0') {
> +	    if ($dst_drivename eq 'efidisk0') {
>  		# the relevant data on the efidisk may be smaller than the source
>  		# e.g. on RBD/ZFS, so we use dd to copy only the amount
>  		# that is given by the OVMF_VARS.fd
> @@ -7647,9 +7656,9 @@ sub clone_disk {
>  		qemu_img_convert($drive->{file}, $newvolid, $size, $snapname, $sparseinit);
>  	    }
>  	} else {

corresponding to this else branch here - mirroring but changing 
drivename is not something we currently ever want to do, so we might 
want to guard against it?

> -	    die "cannot move TPM state while VM is running\n" if $drivename eq 'tpmstate0';
> +	    die "cannot move TPM state while VM is running\n" if $src_drivename eq 'tpmstate0';
>  
> -	    qemu_drive_mirror($vmid, $drivename, $newvolid, $newvmid, $sparseinit, $jobs,
> +	    qemu_drive_mirror($vmid, $src_drivename, $newvolid, $newvmid, $sparseinit, $jobs,
>  	        $completion, $qga, $bwlimit);
>  	}
>      }
> -- 
> 2.30.2
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
> 

Re: [pve-devel] [PATCH v12 qemu-server 07/16] clone disk: allow cloning from an unused or unreferenced disk

[Fabian Ebner]

Am 14.03.22 um 16:55 schrieb Fabian Grünbichler:
> On March 9, 2022 11:09 am, Fabian Ebner wrote:
> corresponding to this else branch here - mirroring but changing 
> drivename is not something we currently ever want to do, so we might 
> want to guard against it?
> 

Actually, it does happen when importing to a different drive key from a
running VM. We don't switch the original drive node in QEMU when the
target VM ID is different (like for regular clone). But there is a
problematic corner case when importing from the same VM to itself ;).
I'll add a check with that considered.

[pve-devel] [PATCH v12 qemu-server 08/16] efivars size: allow overriding efidisk parameter

[Fabian Ebner]

For disk import, it should be based on the disk properties that are
passed in rather than on those of a possibly pre-existing disk in the
config.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

New in v12.

 PVE/QemuServer.pm | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index b98ed3d..b246602 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -7703,9 +7703,10 @@ sub qemu_use_old_bios_files {
 }
 
 sub get_efivars_size {
-    my ($conf) = @_;
+    my ($conf, $efidisk) = @_;
+
     my $arch = get_vm_arch($conf);
-    my $efidisk = $conf->{efidisk0} ? parse_drive('efidisk0', $conf->{efidisk0}) : undef;
+    $efidisk //= $conf->{efidisk0} ? parse_drive('efidisk0', $conf->{efidisk0}) : undef;
     my $smm = PVE::QemuServer::Machine::machine_type_is_q35($conf);
     my (undef, $ovmf_vars) = get_ovmf_files($arch, $efidisk, $smm);
     die "uefi vars image '$ovmf_vars' not found\n" if ! -f $ovmf_vars;
-- 
2.30.2

[pve-devel] [PATCH v12 qemu-server 09/16] schema: add pve-volume-id-or-absolute-path

[Fabian Ebner]

Signed-off-by: Dominic Jäger <d.jaeger@proxmox.com>
[split into its own patch + style fixes]
Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 PVE/QemuServer.pm | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index b246602..33f226e 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -1036,11 +1036,17 @@ PVE::JSONSchema::register_format('pve-volume-id-or-qm-path', \&verify_volume_id_
 sub verify_volume_id_or_qm_path {
     my ($volid, $noerr) = @_;
 
-    if ($volid eq 'none' || $volid eq 'cdrom' || $volid =~ m|^/|) {
-	return $volid;
-    }
+    return $volid if $volid eq 'none' || $volid eq 'cdrom';
+
+    return verify_volume_id_or_absolute_path($volid, $noerr);
+}
+
+PVE::JSONSchema::register_format('pve-volume-id-or-absolute-path', \&verify_volume_id_or_absolute_path);
+sub verify_volume_id_or_absolute_path {
+    my ($volid, $noerr) = @_;
+
+    return $volid if $volid =~ m|^/|;
 
-    # if its neither 'none' nor 'cdrom' nor a path, check if its a volume-id
     $volid = eval { PVE::JSONSchema::check_format('pve-volume-id', $volid, '') };
     if ($@) {
 	return if $noerr;
-- 
2.30.2

[pve-devel] [PATCH v12 qemu-server 10/16] parse ovf: untaint path when calling file_size_info

[Fabian Ebner]

Prepare for calling parse_ovf via API, where the -T switch is used.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 PVE/QemuServer/OVF.pm | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/PVE/QemuServer/OVF.pm b/PVE/QemuServer/OVF.pm
index 0376cbf..b97b052 100644
--- a/PVE/QemuServer/OVF.pm
+++ b/PVE/QemuServer/OVF.pm
@@ -221,10 +221,11 @@ ovf:Item[rasd:InstanceID='%s']/rasd:ResourceType", $controller_id);
 	    die "error parsing $filepath, file seems not to exist at $backing_file_path\n";
 	}
 
-	my $virtual_size;
-	if ( !($virtual_size = PVE::Storage::file_size_info($backing_file_path)) ) {
-	    die "error parsing $backing_file_path, size seems to be $virtual_size\n";
-	}
+	($backing_file_path) = $backing_file_path =~ m|^(/.*)|; # untaint
+
+	my $virtual_size = PVE::Storage::file_size_info($backing_file_path);
+	die "error parsing $backing_file_path, cannot determine file size\n"
+	    if !$virtual_size;
 
 	$pve_disk = {
 	    disk_address => $pve_disk_address,
-- 
2.30.2

[pve-devel] [PATCH v12 qemu-server 11/16] api: add endpoint for parsing .ovf files

[Fabian Ebner]

Co-developed-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
Signed-off-by: Dominic Jäger <d.jaeger@proxmox.com>
[split into its own patch + minor improvements/style fixes]
Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 PVE/API2/Qemu/Makefile |  2 +-
 PVE/API2/Qemu/OVF.pm   | 55 ++++++++++++++++++++++++++++++++++++++++++
 PVE/QemuServer.pm      | 32 ++++++++++++++++++++++++
 3 files changed, 88 insertions(+), 1 deletion(-)
 create mode 100644 PVE/API2/Qemu/OVF.pm

diff --git a/PVE/API2/Qemu/Makefile b/PVE/API2/Qemu/Makefile
index 5d4abda..bdd4762 100644
--- a/PVE/API2/Qemu/Makefile
+++ b/PVE/API2/Qemu/Makefile
@@ -1,4 +1,4 @@
-SOURCES=Agent.pm CPU.pm Machine.pm
+SOURCES=Agent.pm CPU.pm Machine.pm OVF.pm
 
 .PHONY: install
 install:
diff --git a/PVE/API2/Qemu/OVF.pm b/PVE/API2/Qemu/OVF.pm
new file mode 100644
index 0000000..5fa0ef0
--- /dev/null
+++ b/PVE/API2/Qemu/OVF.pm
@@ -0,0 +1,55 @@
+package PVE::API2::Qemu::OVF;
+
+use strict;
+use warnings;
+
+use PVE::JSONSchema qw(get_standard_option);
+use PVE::QemuServer::OVF;
+use PVE::RESTHandler;
+
+use base qw(PVE::RESTHandler);
+
+__PACKAGE__->register_method ({
+    name => 'index',
+    path => '',
+    method => 'GET',
+    proxyto => 'node',
+    description => "Read an .ovf manifest.",
+    parameters => {
+	additionalProperties => 0,
+	properties => {
+	    node => get_standard_option('pve-node'),
+	    manifest => {
+		description => "Path to .ovf manifest.",
+		type => 'string',
+	    },
+	},
+    },
+    returns => {
+	description => "VM config according to .ovf manifest.",
+	type => "object",
+    },
+    returns => {
+	type => 'object',
+	additionalProperties => 1,
+	properties => PVE::QemuServer::json_ovf_properties({}),
+    },
+    code => sub {
+	my ($param) = @_;
+
+	my $manifest = $param->{manifest};
+	die "check for file $manifest failed - $!\n" if !-f $manifest;
+
+	my $parsed = PVE::QemuServer::OVF::parse_ovf($manifest);
+	my $result;
+	$result->{cores} = $parsed->{qm}->{cores};
+	$result->{name} =  $parsed->{qm}->{name};
+	$result->{memory} = $parsed->{qm}->{memory};
+	my $disks = $parsed->{disks};
+	for my $disk (@$disks) {
+	    $result->{$disk->{disk_address}} = $disk->{backing_file};
+	}
+	return $result;
+    }});
+
+1;
diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 33f226e..ecf51f3 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -2203,6 +2203,38 @@ sub json_config_properties {
     return $prop;
 }
 
+# Properties that we can read from an OVF file
+sub json_ovf_properties {
+    my $prop = shift;
+
+    for my $device (PVE::QemuServer::Drive::valid_drive_names()) {
+	$prop->{$device} = {
+	    type => 'string',
+	    format => 'pve-volume-id-or-absolute-path',
+	    description => "Disk image that gets imported to $device",
+	    optional => 1,
+	};
+    }
+
+    $prop->{cores} = {
+	type => 'integer',
+	description => "The number of CPU cores.",
+	optional => 1,
+    };
+    $prop->{memory} = {
+	type => 'integer',
+	description => "Amount of RAM for the VM in MB.",
+	optional => 1,
+    };
+    $prop->{name} = {
+	type => 'string',
+	description => "Name of the VM.",
+	optional => 1,
+    };
+
+    return $prop;
+}
+
 # return copy of $confdesc_cloudinit to generate documentation
 sub cloudinit_config_properties {
 
-- 
2.30.2

Re: [pve-devel] [PATCH v12 qemu-server 11/16] api: add endpoint for parsing .ovf files

[Fabian Grünbichler]

On March 9, 2022 11:09 am, Fabian Ebner wrote:
> Co-developed-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> Signed-off-by: Dominic Jäger <d.jaeger@proxmox.com>
> [split into its own patch + minor improvements/style fixes]
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
>  PVE/API2/Qemu/Makefile |  2 +-
>  PVE/API2/Qemu/OVF.pm   | 55 ++++++++++++++++++++++++++++++++++++++++++
>  PVE/QemuServer.pm      | 32 ++++++++++++++++++++++++
>  3 files changed, 88 insertions(+), 1 deletion(-)
>  create mode 100644 PVE/API2/Qemu/OVF.pm
> 
> diff --git a/PVE/API2/Qemu/Makefile b/PVE/API2/Qemu/Makefile
> index 5d4abda..bdd4762 100644
> --- a/PVE/API2/Qemu/Makefile
> +++ b/PVE/API2/Qemu/Makefile
> @@ -1,4 +1,4 @@
> -SOURCES=Agent.pm CPU.pm Machine.pm
> +SOURCES=Agent.pm CPU.pm Machine.pm OVF.pm
>  
>  .PHONY: install
>  install:
> diff --git a/PVE/API2/Qemu/OVF.pm b/PVE/API2/Qemu/OVF.pm
> new file mode 100644
> index 0000000..5fa0ef0
> --- /dev/null
> +++ b/PVE/API2/Qemu/OVF.pm
> @@ -0,0 +1,55 @@
> +package PVE::API2::Qemu::OVF;
> +
> +use strict;
> +use warnings;
> +
> +use PVE::JSONSchema qw(get_standard_option);
> +use PVE::QemuServer::OVF;
> +use PVE::RESTHandler;
> +
> +use base qw(PVE::RESTHandler);
> +
> +__PACKAGE__->register_method ({
> +    name => 'index',
> +    path => '',
> +    method => 'GET',
> +    proxyto => 'node',
> +    description => "Read an .ovf manifest.",

protected => 1,

else this is limited to files readable by www-data?

also probably should think about how to integrate this into the 
permission system, since being limited to root@pam is rather limiting ;)

e.g., something like a new content type/subdir for importing that 'only' 
requires Datastore.Allocate (not AllocateSpace) or a new priv?

starting off like it is now is of course okay, but we probably want some 
form of nicer import flow for the common cases (like, download OVA from 
URL into import dir on storage foo, then import from there for example).

> +    parameters => {
> +	additionalProperties => 0,
> +	properties => {
> +	    node => get_standard_option('pve-node'),
> +	    manifest => {
> +		description => "Path to .ovf manifest.",
> +		type => 'string',
> +	    },
> +	},
> +    },
> +    returns => {
> +	description => "VM config according to .ovf manifest.",
> +	type => "object",
> +    },
> +    returns => {
> +	type => 'object',
> +	additionalProperties => 1,
> +	properties => PVE::QemuServer::json_ovf_properties({}),
> +    },

duplicate returns key, I guess we want the combination of both here ;)

> +    code => sub {
> +	my ($param) = @_;
> +
> +	my $manifest = $param->{manifest};
> +	die "check for file $manifest failed - $!\n" if !-f $manifest;
> +
> +	my $parsed = PVE::QemuServer::OVF::parse_ovf($manifest);
> +	my $result;
> +	$result->{cores} = $parsed->{qm}->{cores};
> +	$result->{name} =  $parsed->{qm}->{name};
> +	$result->{memory} = $parsed->{qm}->{memory};
> +	my $disks = $parsed->{disks};
> +	for my $disk (@$disks) {
> +	    $result->{$disk->{disk_address}} = $disk->{backing_file};
> +	}
> +	return $result;
> +    }});
> +
> +1;
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index 33f226e..ecf51f3 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -2203,6 +2203,38 @@ sub json_config_properties {
>      return $prop;
>  }
>  
> +# Properties that we can read from an OVF file
> +sub json_ovf_properties {
> +    my $prop = shift;

not sure whether we need the $prop here instead of always starting with 
a clean slate? is there some future extension that uses this that you 
have in mind?

> +
> +    for my $device (PVE::QemuServer::Drive::valid_drive_names()) {
> +	$prop->{$device} = {
> +	    type => 'string',
> +	    format => 'pve-volume-id-or-absolute-path',
> +	    description => "Disk image that gets imported to $device",
> +	    optional => 1,
> +	};
> +    }
> +
> +    $prop->{cores} = {
> +	type => 'integer',
> +	description => "The number of CPU cores.",
> +	optional => 1,
> +    };
> +    $prop->{memory} = {
> +	type => 'integer',
> +	description => "Amount of RAM for the VM in MB.",
> +	optional => 1,
> +    };
> +    $prop->{name} = {
> +	type => 'string',
> +	description => "Name of the VM.",
> +	optional => 1,
> +    };
> +
> +    return $prop;
> +}
> +
>  # return copy of $confdesc_cloudinit to generate documentation
>  sub cloudinit_config_properties {
>  
> -- 
> 2.30.2
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 

Re: [pve-devel] [PATCH v12 qemu-server 11/16] api: add endpoint for parsing .ovf files

[Fabian Ebner]

Am 14.03.22 um 16:55 schrieb Fabian Grünbichler:
> On March 9, 2022 11:09 am, Fabian Ebner wrote:
>> +__PACKAGE__->register_method ({
>> +    name => 'index',
>> +    path => '',
>> +    method => 'GET',
>> +    proxyto => 'node',
>> +    description => "Read an .ovf manifest.",
> 
> protected => 1,
> 
> else this is limited to files readable by www-data?

Ack.

> 
> also probably should think about how to integrate this into the 
> permission system, since being limited to root@pam is rather limiting ;)
> 
> e.g., something like a new content type/subdir for importing that 'only' 
> requires Datastore.Allocate (not AllocateSpace) or a new priv?
> 
> starting off like it is now is of course okay, but we probably want some 
> form of nicer import flow for the common cases (like, download OVA from 
> URL into import dir on storage foo, then import from there for example).
> 

Will revisit when we add that.

>> +    parameters => {
>> +	additionalProperties => 0,
>> +	properties => {
>> +	    node => get_standard_option('pve-node'),
>> +	    manifest => {
>> +		description => "Path to .ovf manifest.",
>> +		type => 'string',
>> +	    },
>> +	},
>> +    },
>> +    returns => {
>> +	description => "VM config according to .ovf manifest.",
>> +	type => "object",
>> +    },
>> +    returns => {
>> +	type => 'object',
>> +	additionalProperties => 1,
>> +	properties => PVE::QemuServer::json_ovf_properties({}),
>> +    },
> 
> duplicate returns key, I guess we want the combination of both here ;)
> 

Will fix it.

>> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
>> index 33f226e..ecf51f3 100644
>> --- a/PVE/QemuServer.pm
>> +++ b/PVE/QemuServer.pm
>> @@ -2203,6 +2203,38 @@ sub json_config_properties {
>>      return $prop;
>>  }
>>  
>> +# Properties that we can read from an OVF file
>> +sub json_ovf_properties {
>> +    my $prop = shift;
> 
> not sure whether we need the $prop here instead of always starting with 
> a clean slate? is there some future extension that uses this that you 
> have in mind?
> 

It was actually used in such a way in Dominic's version, because the
non-drive properties were passed along as $prop. But yes, it can be
dropped now.

[pve-devel] [PATCH v12 qemu-server 12/16] image convert: allow block device as source

[Fabian Ebner]

Necessary to import from an existing storage using block-device
volumes like ZFS.

Signed-off-by: Dominic Jäger <d.jaeger@proxmox.com>
[split into its own patch]
Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 PVE/QemuServer.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index ecf51f3..8c553b1 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -7312,7 +7312,7 @@ sub qemu_img_convert {
 	$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) {
+    } elsif (-f $src_volid || -b $src_volid) {
 	$src_path = $src_volid;
 	if ($src_path =~ m/\.($PVE::QemuServer::Drive::QEMU_FORMAT_RE)$/) {
 	    $src_format = $1;
-- 
2.30.2

[pve-devel] [PATCH v12 qemu-server 13/16] api: factor out check/cleanup for drive params

[Fabian Ebner]

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 PVE/API2/Qemu.pm | 38 +++++++++++++++++++++-----------------
 1 file changed, 21 insertions(+), 17 deletions(-)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 3b86034..7c3bb91 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -63,6 +63,23 @@ my $resolve_cdrom_alias = sub {
     }
 };
 
+my $check_drive_param = sub {
+    my ($param, $storecfg, $extra_checks) = @_;
+
+    for my $opt (sort keys $param->%*) {
+	next if !PVE::QemuServer::is_valid_drivename($opt);
+
+	my $drive = PVE::QemuServer::parse_drive($opt, $param->{$opt});
+	raise_param_exc({ $opt => "unable to parse drive options" }) if !$drive;
+
+	PVE::QemuServer::cleanup_drive_path($opt, $storecfg, $drive);
+
+	$extra_checks->($drive) if $extra_checks;
+
+	$param->{$opt} = PVE::QemuServer::print_drive($drive);
+    }
+};
+
 my $NEW_DISK_RE = qr!^(([^/:\s]+):)?(\d+(\.\d+)?)$!;
 my $check_storage_access = sub {
    my ($rpcenv, $authuser, $storecfg, $vmid, $settings, $default_storage) = @_;
@@ -617,15 +634,7 @@ __PACKAGE__->register_method({
 
 	    &$check_cpu_model_access($rpcenv, $authuser, $param);
 
-	    foreach my $opt (keys %$param) {
-		if (PVE::QemuServer::is_valid_drivename($opt)) {
-		    my $drive = PVE::QemuServer::parse_drive($opt, $param->{$opt});
-		    raise_param_exc({ $opt => "unable to parse drive options" }) if !$drive;
-
-		    PVE::QemuServer::cleanup_drive_path($opt, $storecfg, $drive);
-		    $param->{$opt} = PVE::QemuServer::print_drive($drive);
-		}
-	    }
+	    $check_drive_param->($param, $storecfg);
 
 	    PVE::QemuServer::add_random_macs($param);
 	} else {
@@ -1195,15 +1204,10 @@ my $update_vm_api  = sub {
 	die "cannot add non-replicatable volume to a replicated VM\n";
     };
 
+    $check_drive_param->($param, $storecfg, $check_replication);
+
     foreach my $opt (keys %$param) {
-	if (PVE::QemuServer::is_valid_drivename($opt)) {
-	    # cleanup drive path
-	    my $drive = PVE::QemuServer::parse_drive($opt, $param->{$opt});
-	    raise_param_exc({ $opt => "unable to parse drive options" }) if !$drive;
-	    PVE::QemuServer::cleanup_drive_path($opt, $storecfg, $drive);
-	    $check_replication->($drive);
-	    $param->{$opt} = PVE::QemuServer::print_drive($drive);
-	} elsif ($opt =~ m/^net(\d+)$/) {
+	if ($opt =~ m/^net(\d+)$/) {
 	    # add macaddr
 	    my $net = PVE::QemuServer::parse_net($param->{$opt});
 	    $param->{$opt} = PVE::QemuServer::print_net($net);
-- 
2.30.2

[pve-devel] [PATCH v12 qemu-server 14/16] schema: drive: use separate schema when disk allocation is possible

[Fabian Ebner]

via the special syntax <storeid>:<size>.

Not worth it by itself, but this is anticipating a new 'import-from'
parameter which is only used upon import/allocation, but shouldn't be
part of the schema for the config or other API enpoints.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 PVE/API2/Qemu.pm        | 12 ++++++--
 PVE/QemuServer.pm       |  9 ++++--
 PVE/QemuServer/Drive.pm | 62 +++++++++++++++++++++++++++++------------
 3 files changed, 60 insertions(+), 23 deletions(-)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 7c3bb91..216c326 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -569,7 +569,9 @@ __PACKAGE__->register_method({
 		    default => 0,
 		    description => "Start VM after it was created successfully.",
 		},
-	    }),
+	    },
+	    1, # with_disk_alloc
+	),
     },
     returns => {
 	type => 'string',
@@ -1552,7 +1554,9 @@ __PACKAGE__->register_method({
 		    maximum => 30,
 		    optional => 1,
 		},
-	    }),
+	    },
+	    1, # with_disk_alloc
+	),
     },
     returns => {
 	type => 'string',
@@ -1600,7 +1604,9 @@ __PACKAGE__->register_method({
 		    maxLength => 40,
 		    optional => 1,
 		},
-	    }),
+	    },
+	    1, # with_disk_alloc
+	),
     },
     returns => { type => 'null' },
     code => sub {
diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 8c553b1..5b68bdf 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -2184,7 +2184,7 @@ sub verify_usb_device {
 
 # add JSON properties for create and set function
 sub json_config_properties {
-    my $prop = shift;
+    my ($prop, $with_disk_alloc) = @_;
 
     my $skip_json_config_opts = {
 	parent => 1,
@@ -2197,7 +2197,12 @@ sub json_config_properties {
 
     foreach my $opt (keys %$confdesc) {
 	next if $skip_json_config_opts->{$opt};
-	$prop->{$opt} = $confdesc->{$opt};
+
+	if ($with_disk_alloc && is_valid_drivename($opt)) {
+	    $prop->{$opt} = $PVE::QemuServer::Drive::drivedesc_hash_with_alloc->{$opt};
+	} else {
+	    $prop->{$opt} = $confdesc->{$opt};
+	}
     }
 
     return $prop;
diff --git a/PVE/QemuServer/Drive.pm b/PVE/QemuServer/Drive.pm
index 7b82fb2..d5d4723 100644
--- a/PVE/QemuServer/Drive.pm
+++ b/PVE/QemuServer/Drive.pm
@@ -3,6 +3,8 @@ package PVE::QemuServer::Drive;
 use strict;
 use warnings;
 
+use Storable qw(dclone);
+
 use PVE::Storage;
 use PVE::JSONSchema qw(get_standard_option);
 
@@ -33,6 +35,8 @@ our $MAX_SATA_DISKS = 6;
 our $MAX_UNUSED_DISKS = 256;
 
 our $drivedesc_hash;
+# Schema when disk allocation is possible.
+our $drivedesc_hash_with_alloc = {};
 
 my %drivedesc_base = (
     volume => { alias => 'file' },
@@ -262,14 +266,10 @@ my $ide_fmt = {
 };
 PVE::JSONSchema::register_format("pve-qm-ide", $ide_fmt);
 
-my $ALLOCATION_SYNTAX_DESC =
-    "Use the special syntax STORAGE_ID:SIZE_IN_GiB to allocate a new volume.";
-
 my $idedesc = {
     optional => 1,
     type => 'string', format => $ide_fmt,
-    description => "Use volume as IDE hard disk or CD-ROM (n is 0 to " .($MAX_IDE_DISKS -1) . "). " .
-	$ALLOCATION_SYNTAX_DESC,
+    description => "Use volume as IDE hard disk or CD-ROM (n is 0 to " .($MAX_IDE_DISKS - 1) . ").",
 };
 PVE::JSONSchema::register_standard_option("pve-qm-ide", $idedesc);
 
@@ -285,8 +285,7 @@ my $scsi_fmt = {
 my $scsidesc = {
     optional => 1,
     type => 'string', format => $scsi_fmt,
-    description => "Use volume as SCSI hard disk or CD-ROM (n is 0 to " . ($MAX_SCSI_DISKS - 1) . "). " .
-	$ALLOCATION_SYNTAX_DESC,
+    description => "Use volume as SCSI hard disk or CD-ROM (n is 0 to " . ($MAX_SCSI_DISKS - 1) . ").",
 };
 PVE::JSONSchema::register_standard_option("pve-qm-scsi", $scsidesc);
 
@@ -298,8 +297,7 @@ my $sata_fmt = {
 my $satadesc = {
     optional => 1,
     type => 'string', format => $sata_fmt,
-    description => "Use volume as SATA hard disk or CD-ROM (n is 0 to " . ($MAX_SATA_DISKS - 1). "). " .
-	$ALLOCATION_SYNTAX_DESC,
+    description => "Use volume as SATA hard disk or CD-ROM (n is 0 to " . ($MAX_SATA_DISKS - 1). ").",
 };
 PVE::JSONSchema::register_standard_option("pve-qm-sata", $satadesc);
 
@@ -311,8 +309,7 @@ my $virtio_fmt = {
 my $virtiodesc = {
     optional => 1,
     type => 'string', format => $virtio_fmt,
-    description => "Use volume as VIRTIO hard disk (n is 0 to " . ($MAX_VIRTIO_DISKS - 1) . "). " .
-	$ALLOCATION_SYNTAX_DESC,
+    description => "Use volume as VIRTIO hard disk (n is 0 to " . ($MAX_VIRTIO_DISKS - 1) . ").",
 };
 PVE::JSONSchema::register_standard_option("pve-qm-virtio", $virtiodesc);
 
@@ -359,9 +356,7 @@ my $efidisk_fmt = {
 my $efidisk_desc = {
     optional => 1,
     type => 'string', format => $efidisk_fmt,
-    description => "Configure a Disk for storing EFI vars. " .
-	$ALLOCATION_SYNTAX_DESC . " Note that SIZE_IN_GiB is ignored here " .
-	"and that the default EFI vars are copied to the volume instead.",
+    description => "Configure a Disk for storing EFI vars.",
 };
 
 PVE::JSONSchema::register_standard_option("pve-qm-efidisk", $efidisk_desc);
@@ -397,10 +392,7 @@ my $tpmstate_fmt = {
 my $tpmstate_desc = {
     optional => 1,
     type => 'string', format => $tpmstate_fmt,
-    description => "Configure a Disk for storing TPM state. " .
-	$ALLOCATION_SYNTAX_DESC . " Note that SIZE_IN_GiB is ignored here " .
-	"and that the default size of 4 MiB will always be used instead. The " .
-	"format is also fixed to 'raw'.",
+    description => "Configure a Disk for storing TPM state. The format is fixed to 'raw'.",
 };
 use constant TPMSTATE_DISK_SIZE => 4 * 1024 * 1024;
 
@@ -434,27 +426,61 @@ my $unuseddesc = {
     description => "Reference to unused volumes. This is used internally, and should not be modified manually.",
 };
 
+my $with_alloc_desc_cache = {
+    unused => $unuseddesc, # Allocation for unused is not supported currently.
+};
+my $desc_with_alloc = sub {
+    my ($type, $desc) = @_;
+
+    return $with_alloc_desc_cache->{$type} if $with_alloc_desc_cache->{$type};
+
+    my $new_desc = dclone($desc);
+
+    my $extra_note = '';
+    if ($type eq 'efidisk') {
+	$extra_note = " Note that SIZE_IN_GiB is ignored here and that the default EFI vars are ".
+	    "copied to the volume instead.";
+    } elsif ($type eq 'tpmstate') {
+	$extra_note = " Note that SIZE_IN_GiB is ignored here and 4 MiB will be used instead.";
+    }
+
+    $new_desc->{description} .= " Use the special syntax STORAGE_ID:SIZE_IN_GiB to allocate a new ".
+	"volume.${extra_note}";
+
+    $with_alloc_desc_cache->{$type} = $new_desc;
+
+    return $new_desc;
+};
+
 for (my $i = 0; $i < $MAX_IDE_DISKS; $i++)  {
     $drivedesc_hash->{"ide$i"} = $idedesc;
+    $drivedesc_hash_with_alloc->{"ide$i"} = $desc_with_alloc->('ide', $idedesc);
 }
 
 for (my $i = 0; $i < $MAX_SATA_DISKS; $i++)  {
     $drivedesc_hash->{"sata$i"} = $satadesc;
+    $drivedesc_hash_with_alloc->{"sata$i"} = $desc_with_alloc->('sata', $satadesc);
 }
 
 for (my $i = 0; $i < $MAX_SCSI_DISKS; $i++)  {
     $drivedesc_hash->{"scsi$i"} = $scsidesc;
+    $drivedesc_hash_with_alloc->{"scsi$i"} = $desc_with_alloc->('scsi', $scsidesc);
 }
 
 for (my $i = 0; $i < $MAX_VIRTIO_DISKS; $i++)  {
     $drivedesc_hash->{"virtio$i"} = $virtiodesc;
+    $drivedesc_hash_with_alloc->{"virtio$i"} = $desc_with_alloc->('virtio', $virtiodesc);
 }
 
 $drivedesc_hash->{efidisk0} = $efidisk_desc;
+$drivedesc_hash_with_alloc->{efidisk0} = $desc_with_alloc->('efidisk', $efidisk_desc);
+
 $drivedesc_hash->{tpmstate0} = $tpmstate_desc;
+$drivedesc_hash_with_alloc->{tpmstate0} = $desc_with_alloc->('tpmstate', $tpmstate_desc);
 
 for (my $i = 0; $i < $MAX_UNUSED_DISKS; $i++) {
     $drivedesc_hash->{"unused$i"} = $unuseddesc;
+    $drivedesc_hash_with_alloc->{"unused$i"} = $desc_with_alloc->('unused', $unuseddesc);
 }
 
 sub valid_drive_names_for_boot {
-- 
2.30.2

[pve-devel] [PATCH v12 qemu-server 15/16] api: support VM disk import

[Fabian Ebner]

From: Dominic Jäger <d.jaeger@proxmox.com>

Extend qm importdisk functionality to the API.

Co-authored-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
Co-authored-by: Dominic Jäger <d.jaeger@proxmox.com>
Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

Changes from v11:
    * Require relevant parameters to be set explicitly for EFI/TPM
      disk import.
    * Base calculation of EFI vars size on passed-in parameters.

 PVE/API2/Qemu.pm             | 229 ++++++++++++++++++++++++++++++-----
 PVE/QemuServer/Drive.pm      |  34 +++++-
 PVE/QemuServer/ImportDisk.pm |   2 +-
 3 files changed, 230 insertions(+), 35 deletions(-)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 216c326..9220ce2 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -21,8 +21,9 @@ use PVE::ReplicationConfig;
 use PVE::GuestHelpers;
 use PVE::QemuConfig;
 use PVE::QemuServer;
-use PVE::QemuServer::Drive;
 use PVE::QemuServer::CPUConfig;
+use PVE::QemuServer::Drive;
+use PVE::QemuServer::ImportDisk;
 use PVE::QemuServer::Monitor qw(mon_cmd);
 use PVE::QemuServer::Machine;
 use PVE::QemuMigrate;
@@ -63,28 +64,58 @@ my $resolve_cdrom_alias = sub {
     }
 };
 
+# Used in import-enabled API endpoints. Parses drives using the extended '_with_alloc' schema.
+my $foreach_volume_with_alloc = sub {
+    my ($param, $func) = @_;
+
+    for my $opt (sort keys $param->%*) {
+	next if !PVE::QemuServer::is_valid_drivename($opt);
+
+	my $drive = PVE::QemuServer::Drive::parse_drive($opt, $param->{$opt}, 1);
+	next if !$drive;
+
+	$func->($opt, $drive);
+    }
+};
+
+my $NEW_DISK_RE = qr!^(([^/:\s]+):)?(\d+(\.\d+)?)$!;
+
 my $check_drive_param = sub {
     my ($param, $storecfg, $extra_checks) = @_;
 
     for my $opt (sort keys $param->%*) {
 	next if !PVE::QemuServer::is_valid_drivename($opt);
 
-	my $drive = PVE::QemuServer::parse_drive($opt, $param->{$opt});
+	my $drive = PVE::QemuServer::parse_drive($opt, $param->{$opt}, 1);
 	raise_param_exc({ $opt => "unable to parse drive options" }) if !$drive;
 
+	if ($drive->{'import-from'}) {
+	    die "'import-from' requires special syntax - use <storage ID>:0,import-from=<source>\n"
+		if $drive->{file} !~ $NEW_DISK_RE || $3 != 0;
+
+	    if ($opt eq 'efidisk0') {
+		for my $required (qw(efitype pre-enrolled-keys)) {
+		    die "$opt - need to specify '$required' when using 'import-from'\n"
+			if !defined($drive->{$required});
+		}
+	    } elsif ($opt eq 'tpmstate0') {
+		die "$opt - need to specify 'version' when using 'import-from'\n"
+		    if !defined($drive->{version});
+	    }
+	}
+
 	PVE::QemuServer::cleanup_drive_path($opt, $storecfg, $drive);
 
 	$extra_checks->($drive) if $extra_checks;
 
-	$param->{$opt} = PVE::QemuServer::print_drive($drive);
+	$param->{$opt} = PVE::QemuServer::print_drive($drive, 1);
     }
 };
 
-my $NEW_DISK_RE = qr!^(([^/:\s]+):)?(\d+(\.\d+)?)$!;
 my $check_storage_access = sub {
    my ($rpcenv, $authuser, $storecfg, $vmid, $settings, $default_storage) = @_;
 
-   PVE::QemuConfig->foreach_volume($settings, sub {
+   $foreach_volume_with_alloc->($settings, sub {
 	my ($ds, $drive) = @_;
 
 	my $isCDROM = PVE::QemuServer::drive_is_cdrom($drive);
@@ -106,6 +137,20 @@ my $check_storage_access = sub {
 	} else {
 	    PVE::Storage::check_volume_access($rpcenv, $authuser, $storecfg, $vmid, $volid);
 	}
+
+	if (my $src_image = $drive->{'import-from'}) {
+	    my $src_vmid;
+	    my ($src_storeid) = PVE::Storage::parse_volume_id($src_image, 1);
+	    if ($src_storeid) { # PVE-managed volume
+		$src_vmid = (PVE::Storage::parse_volname($storecfg, $src_image))[2]
+	    }
+
+	    if ($src_vmid) { # might be actively used by VM and will be copied via clone_disk()
+		$rpcenv->check($authuser, "/vms/${src_vmid}", ['VM.Clone']);
+	    } else {
+		PVE::Storage::check_volume_access($rpcenv, $authuser, $storecfg, $vmid, $src_image);
+	    }
+	}
     });
 
    $rpcenv->check($authuser, "/storage/$settings->{vmstatestorage}", ['Datastore.AllocateSpace'])
@@ -164,6 +209,87 @@ my $check_storage_access_migrate = sub {
 	if !$scfg->{content}->{images};
 };
 
+my $import_from_volid = sub {
+    my ($storecfg, $src_volid, $dest_info, $vollist) = @_;
+
+    die "cannot import from cloudinit disk\n"
+	if PVE::QemuServer::Drive::drive_is_cloudinit({ file => $src_volid });
+
+    my ($src_storeid, $src_volname) = PVE::Storage::parse_volume_id($src_volid);
+    my $src_vmid = (PVE::Storage::parse_volname($storecfg, $src_volid))[2];
+
+    my $src_vm_state = sub {
+	my $exists = $src_vmid && PVE::Cluster::get_vmlist()->{ids}->{$src_vmid} ? 1 : 0;
+
+	my $runs = 0;
+	if ($exists) {
+	    eval { PVE::QemuConfig::assert_config_exists_on_node($src_vmid); };
+	    die "owner VM $src_vmid not on local node\n" if $@;
+	    $runs = PVE::QemuServer::Helpers::vm_running_locally($src_vmid) || 0;
+	}
+
+	return ($exists, $runs);
+    };
+
+    my ($src_vm_exists, $running) = $src_vm_state->();
+
+    die "cannot import from '$src_volid' - full clone feature is not supported\n"
+	if !PVE::Storage::volume_has_feature($storecfg, 'copy', $src_volid, undef, $running);
+
+    my $clonefn = sub {
+	my ($src_vm_exists_now, $running_now) = $src_vm_state->();
+
+	die "owner VM $src_vmid changed state unexpectedly\n"
+	    if $src_vm_exists_now != $src_vm_exists || $running_now != $running;
+
+	my $src_conf = $src_vm_exists_now ? PVE::QemuConfig->load_config($src_vmid) : {};
+
+	my $src_drive = { file => $src_volid };
+	my $src_drivename;
+	PVE::QemuConfig->foreach_volume($src_conf, sub {
+	    my ($ds, $drive) = @_;
+
+	    return if $src_drivename;
+
+	    if ($drive->{file} eq $src_volid) {
+		$src_drive = $drive;
+		$src_drivename = $ds;
+	    }
+	});
+
+	my $source_info = {
+	    vmid => $src_vmid,
+	    running => $running_now,
+	    drivename => $src_drivename,
+	    drive => $src_drive,
+	    snapname => undef,
+	};
+
+	return PVE::QemuServer::clone_disk(
+	    $storecfg,
+	    $source_info,
+	    $dest_info,
+	    1,
+	    $vollist,
+	    undef,
+	    undef,
+	    $src_conf->{agent},
+	    PVE::Storage::get_bandwidth_limit('clone', [$src_storeid, $dest_info->{storage}]),
+	);
+    };
+
+    my $cloned;
+    if ($running) {
+	$cloned = PVE::QemuConfig->lock_config_full($src_vmid, 30, $clonefn);
+    } elsif ($src_vmid) {
+	$cloned = PVE::QemuConfig->lock_config_shared($src_vmid, 30, $clonefn);
+    } else {
+	$cloned = $clonefn->();
+    }
+
+    return $cloned->@{qw(file size)};
+};
+
 # Note: $pool is only needed when creating a VM, because pool permissions
 # are automatically inherited if VM already exists inside a pool.
 my $create_disks = sub {
@@ -207,28 +333,75 @@ my $create_disks = sub {
 	} elsif ($volid =~ $NEW_DISK_RE) {
 	    my ($storeid, $size) = ($2 || $default_storage, $3);
 	    die "no storage ID specified (and no default storage)\n" if !$storeid;
-	    my $defformat = PVE::Storage::storage_default_format($storecfg, $storeid);
-	    my $fmt = $disk->{format} || $defformat;
-
-	    $size = PVE::Tools::convert_size($size, 'gb' => 'kb'); # vdisk_alloc uses kb
-
-	    my $volid;
-	    if ($ds eq 'efidisk0') {
-		my $smm = PVE::QemuServer::Machine::machine_type_is_q35($conf);
-		($volid, $size) = PVE::QemuServer::create_efidisk(
-		    $storecfg, $storeid, $vmid, $fmt, $arch, $disk, $smm);
-	    } elsif ($ds eq 'tpmstate0') {
-		# swtpm can only use raw volumes, and uses a fixed size
-		$size = PVE::Tools::convert_size(PVE::QemuServer::Drive::TPMSTATE_DISK_SIZE, 'b' => 'kb');
-		$volid = PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, "raw", undef, $size);
+
+	    if (my $source = delete $disk->{'import-from'}) {
+		my $dst_volid;
+		my ($src_storeid) = PVE::Storage::parse_volume_id($source, 1);
+
+		if ($src_storeid) { # PVE-managed volume
+		    die "could not get size of $source\n"
+			if !PVE::Storage::volume_size_info($storecfg, $source, 10);
+
+		    my $dest_info = {
+			vmid => $vmid,
+			drivename => $ds,
+			storage => $storeid,
+			format => $disk->{format},
+		    };
+
+		    $dest_info->{efisize} = PVE::QemuServer::get_efivars_size($conf, $disk)
+			if $ds eq 'efidisk0';
+
+		    ($dst_volid, $size) = eval {
+			$import_from_volid->($storecfg, $source, $dest_info, $vollist);
+		    };
+		    die "cannot import from '$source' - $@" if $@;
+		} else {
+		    $source = PVE::Storage::abs_filesystem_path($storecfg, $source, 1);
+		    $size = PVE::Storage::file_size_info($source);
+		    die "could not get file size of $source\n" if !$size;
+
+		    (undef, $dst_volid) = PVE::QemuServer::ImportDisk::do_import(
+			$source,
+			$vmid,
+			$storeid,
+			{
+			    drive_name => $ds,
+			    format => $disk->{format},
+			    'skip-config-update' => 1,
+			},
+		    );
+		    push @$vollist, $dst_volid;
+		}
+
+		$disk->{file} = $dst_volid;
+		$disk->{size} = $size;
+		delete $disk->{format}; # no longer needed
+		$res->{$ds} = PVE::QemuServer::print_drive($disk);
 	    } else {
-		$volid = PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, $fmt, undef, $size);
+		my $defformat = PVE::Storage::storage_default_format($storecfg, $storeid);
+		my $fmt = $disk->{format} || $defformat;
+
+		$size = PVE::Tools::convert_size($size, 'gb' => 'kb'); # vdisk_alloc uses kb
+
+		my $volid;
+		if ($ds eq 'efidisk0') {
+		    my $smm = PVE::QemuServer::Machine::machine_type_is_q35($conf);
+		    ($volid, $size) = PVE::QemuServer::create_efidisk(
+			$storecfg, $storeid, $vmid, $fmt, $arch, $disk, $smm);
+		} elsif ($ds eq 'tpmstate0') {
+		    # swtpm can only use raw volumes, and uses a fixed size
+		    $size = PVE::Tools::convert_size(PVE::QemuServer::Drive::TPMSTATE_DISK_SIZE, 'b' => 'kb');
+		    $volid = PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, "raw", undef, $size);
+		} else {
+		    $volid = PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, $fmt, undef, $size);
+		}
+		push @$vollist, $volid;
+		$disk->{file} = $volid;
+		$disk->{size} = PVE::Tools::convert_size($size, 'kb' => 'b');
+		delete $disk->{format}; # no longer needed
+		$res->{$ds} = PVE::QemuServer::print_drive($disk);
 	    }
-	    push @$vollist, $volid;
-	    $disk->{file} = $volid;
-	    $disk->{size} = PVE::Tools::convert_size($size, 'kb' => 'b');
-	    delete $disk->{format}; # no longer needed
-	    $res->{$ds} = PVE::QemuServer::print_drive($disk);
 	} else {
 	    PVE::Storage::check_volume_access($rpcenv, $authuser, $storecfg, $vmid, $volid);
 
@@ -242,7 +415,7 @@ my $create_disks = sub {
 	}
     };
 
-    eval { PVE::QemuConfig->foreach_volume($settings, $code); };
+    eval { $foreach_volume_with_alloc->($settings, $code); };
 
     # free allocated images on error
     if (my $err = $@) {
@@ -1285,7 +1458,7 @@ my $update_vm_api  = sub {
 
 	    my $check_drive_perms = sub {
 		my ($opt, $val) = @_;
-		my $drive = PVE::QemuServer::parse_drive($opt, $val);
+		my $drive = PVE::QemuServer::parse_drive($opt, $val, 1);
 		# FIXME: cloudinit: CDROM or Disk?
 		if (PVE::QemuServer::drive_is_cdrom($drive)) { # CDROM
 		    $rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.CDROM']);
@@ -1391,7 +1564,7 @@ my $update_vm_api  = sub {
 		    # default legacy boot order implies all cdroms anyway
 		    if (@bootorder) {
 			# append new CD drives to bootorder to mark them bootable
-			my $drive = PVE::QemuServer::parse_drive($opt, $param->{$opt});
+			my $drive = PVE::QemuServer::parse_drive($opt, $param->{$opt}, 1);
 			if (PVE::QemuServer::drive_is_cdrom($drive, 1) && !grep(/^$opt$/, @bootorder)) {
 			    push @bootorder, $opt;
 			    $conf->{pending}->{boot} = PVE::QemuServer::print_bootorder(\@bootorder);
diff --git a/PVE/QemuServer/Drive.pm b/PVE/QemuServer/Drive.pm
index d5d4723..88f013a 100644
--- a/PVE/QemuServer/Drive.pm
+++ b/PVE/QemuServer/Drive.pm
@@ -409,6 +409,22 @@ my $alldrive_fmt = {
     %efitype_fmt,
 };
 
+my %import_from_fmt = (
+    'import-from' => {
+	type => 'string',
+	format => 'pve-volume-id-or-absolute-path',
+	format_description => 'source volume',
+	description => "Create a new disk, importing from this source. If the volume is not ".
+	    "managed by Proxmox VE, it's up to you to ensure that it's not actively used by ".
+	    "another process during the import!",
+	optional => 1,
+    },
+);
+my $alldrive_fmt_with_alloc = {
+    %$alldrive_fmt,
+    %import_from_fmt,
+};
+
 my $unused_fmt = {
     volume => { alias => 'file' },
     file => {
@@ -436,6 +452,8 @@ my $desc_with_alloc = sub {
 
     my $new_desc = dclone($desc);
 
+    $new_desc->{format}->{'import-from'} = $import_from_fmt{'import-from'};
+
     my $extra_note = '';
     if ($type eq 'efidisk') {
 	$extra_note = " Note that SIZE_IN_GiB is ignored here and that the default EFI vars are ".
@@ -445,7 +463,8 @@ my $desc_with_alloc = sub {
     }
 
     $new_desc->{description} .= " Use the special syntax STORAGE_ID:SIZE_IN_GiB to allocate a new ".
-	"volume.${extra_note}";
+	"volume.${extra_note} Use STORAGE_ID:0 and the 'import-from' parameter to import from an ".
+	"existing volume.";
 
     $with_alloc_desc_cache->{$type} = $new_desc;
 
@@ -547,7 +566,7 @@ sub drive_is_read_only {
 #        [,iothread=on][,serial=serial][,model=model]
 
 sub parse_drive {
-    my ($key, $data) = @_;
+    my ($key, $data, $with_alloc) = @_;
 
     my ($interface, $index);
 
@@ -558,12 +577,14 @@ sub parse_drive {
 	return;
     }
 
-    if (!defined($drivedesc_hash->{$key})) {
+    my $desc_hash = $with_alloc ? $drivedesc_hash_with_alloc : $drivedesc_hash;
+
+    if (!defined($desc_hash->{$key})) {
 	warn "invalid drive key: $key\n";
 	return;
     }
 
-    my $desc = $drivedesc_hash->{$key}->{format};
+    my $desc = $desc_hash->{$key}->{format};
     my $res = eval { PVE::JSONSchema::parse_property_string($desc, $data) };
     return if !$res;
     $res->{interface} = $interface;
@@ -623,9 +644,10 @@ sub parse_drive {
 }
 
 sub print_drive {
-    my ($drive) = @_;
+    my ($drive, $with_alloc) = @_;
     my $skip = [ 'index', 'interface' ];
-    return PVE::JSONSchema::print_property_string($drive, $alldrive_fmt, $skip);
+    my $fmt = $with_alloc ? $alldrive_fmt_with_alloc : $alldrive_fmt;
+    return PVE::JSONSchema::print_property_string($drive, $fmt, $skip);
 }
 
 sub get_bootdisks {
diff --git a/PVE/QemuServer/ImportDisk.pm b/PVE/QemuServer/ImportDisk.pm
index 51ad52e..7557cac 100755
--- a/PVE/QemuServer/ImportDisk.pm
+++ b/PVE/QemuServer/ImportDisk.pm
@@ -71,7 +71,7 @@ sub do_import {
 	PVE::Storage::activate_volumes($storecfg, [$dst_volid]);
 	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);
+	PVE::QemuConfig->lock_config($vmid, $create_drive) if !$params->{'skip-config-update'};
     };
     if (my $err = $@) {
 	eval { PVE::Storage::vdisk_free($storecfg, $dst_volid) };
-- 
2.30.2

Re: [pve-devel] [PATCH v12 qemu-server 15/16] api: support VM disk import

[Fabian Grünbichler]

On March 9, 2022 11:09 am, Fabian Ebner wrote:
> From: Dominic Jäger <d.jaeger@proxmox.com>
> 
> Extend qm importdisk functionality to the API.
> 
> Co-authored-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> Co-authored-by: Dominic Jäger <d.jaeger@proxmox.com>
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
> 
> Changes from v11:
>     * Require relevant parameters to be set explicitly for EFI/TPM
>       disk import.
>     * Base calculation of EFI vars size on passed-in parameters.
> 
>  PVE/API2/Qemu.pm             | 229 ++++++++++++++++++++++++++++++-----
>  PVE/QemuServer/Drive.pm      |  34 +++++-
>  PVE/QemuServer/ImportDisk.pm |   2 +-
>  3 files changed, 230 insertions(+), 35 deletions(-)
> 
> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
> index 216c326..9220ce2 100644
> --- a/PVE/API2/Qemu.pm
> +++ b/PVE/API2/Qemu.pm
> @@ -21,8 +21,9 @@ use PVE::ReplicationConfig;
>  use PVE::GuestHelpers;
>  use PVE::QemuConfig;
>  use PVE::QemuServer;
> -use PVE::QemuServer::Drive;
>  use PVE::QemuServer::CPUConfig;
> +use PVE::QemuServer::Drive;
> +use PVE::QemuServer::ImportDisk;
>  use PVE::QemuServer::Monitor qw(mon_cmd);
>  use PVE::QemuServer::Machine;
>  use PVE::QemuMigrate;
> @@ -63,28 +64,58 @@ my $resolve_cdrom_alias = sub {
>      }
>  };
>  
> +# Used in import-enabled API endpoints. Parses drives using the extended '_with_alloc' schema.
> +my $foreach_volume_with_alloc = sub {
> +    my ($param, $func) = @_;
> +
> +    for my $opt (sort keys $param->%*) {
> +	next if !PVE::QemuServer::is_valid_drivename($opt);
> +
> +	my $drive = PVE::QemuServer::Drive::parse_drive($opt, $param->{$opt}, 1);
> +	next if !$drive;
> +
> +	$func->($opt, $drive);
> +    }
> +};
> +
> +my $NEW_DISK_RE = qr!^(([^/:\s]+):)?(\d+(\.\d+)?)$!;
> +
>  my $check_drive_param = sub {
>      my ($param, $storecfg, $extra_checks) = @_;
>  
>      for my $opt (sort keys $param->%*) {
>  	next if !PVE::QemuServer::is_valid_drivename($opt);
>  
> -	my $drive = PVE::QemuServer::parse_drive($opt, $param->{$opt});
> +	my $drive = PVE::QemuServer::parse_drive($opt, $param->{$opt}, 1);

technically belongs into the previous patch, our non-alloc schema is 
just tolerant enough because it doesn't look at the volids too closely 
and accepts the NEW_DISK_RE syntax as potential existing volid..

>  	raise_param_exc({ $opt => "unable to parse drive options" }) if !$drive;
>  
> +	if ($drive->{'import-from'}) {
> +	    die "'import-from' requires special syntax - use <storage ID>:0,import-from=<source>\n"
> +		if $drive->{file} !~ $NEW_DISK_RE || $3 != 0;

should probably be a param_exc

> +
> +	    if ($opt eq 'efidisk0') {
> +		for my $required (qw(efitype pre-enrolled-keys)) {
> +		    die "$opt - need to specify '$required' when using 'import-from'\n"
> +			if !defined($drive->{$required});

same here

> +		}
> +	    } elsif ($opt eq 'tpmstate0') {
> +		die "$opt - need to specify 'version' when using 'import-from'\n"
> +		    if !defined($drive->{version});

and here

> +	    }
> +	}
> +
>  	PVE::QemuServer::cleanup_drive_path($opt, $storecfg, $drive);
>  
>  	$extra_checks->($drive) if $extra_checks;
>  
> -	$param->{$opt} = PVE::QemuServer::print_drive($drive);
> +	$param->{$opt} = PVE::QemuServer::print_drive($drive, 1);
>      }
>  };
>  
> -my $NEW_DISK_RE = qr!^(([^/:\s]+):)?(\d+(\.\d+)?)$!;
>  my $check_storage_access = sub {
>     my ($rpcenv, $authuser, $storecfg, $vmid, $settings, $default_storage) = @_;
>  
> -   PVE::QemuConfig->foreach_volume($settings, sub {
> +   $foreach_volume_with_alloc->($settings, sub {
>  	my ($ds, $drive) = @_;
>  
>  	my $isCDROM = PVE::QemuServer::drive_is_cdrom($drive);
> @@ -106,6 +137,20 @@ my $check_storage_access = sub {
>  	} else {
>  	    PVE::Storage::check_volume_access($rpcenv, $authuser, $storecfg, $vmid, $volid);
>  	}
> +
> +	if (my $src_image = $drive->{'import-from'}) {
> +	    my $src_vmid;
> +	    my ($src_storeid) = PVE::Storage::parse_volume_id($src_image, 1);
> +	    if ($src_storeid) { # PVE-managed volume

nit, could be

if (PVE::Storage::parse_volume_id($src_image, 1)) { # PVE-managed

since we don't actually care about the sid here, and parse_volume_id 
will return undef when $noerr is set.

> +		$src_vmid = (PVE::Storage::parse_volname($storecfg, $src_image))[2]

is there some case where we expect parse_volume_id to work, but the 
volume to not have an associated guest? because perl doesn't mind us 
accessing the resulting array at arbitrary indices, so this doesn't fail 
if $src_vmid is undef..

these should probably also check some more stuff (at least the volume 
type?) - else we get strange errors when attempting to import 
non-image-volumes (some of which even have owners, for example backup 
archives..), and what exactly gets caught where is basically up to the 
storage plugin via parse_volname and volume_has_feature..

> +	    }
> +
> +	    if ($src_vmid) { # might be actively used by VM and will be copied via clone_disk()
> +		$rpcenv->check($authuser, "/vms/${src_vmid}", ['VM.Clone']);
> +	    } else {
> +		PVE::Storage::check_volume_access($rpcenv, $authuser, $storecfg, $vmid, $src_image);
> +	    }
> +	}
>      });
>  
>     $rpcenv->check($authuser, "/storage/$settings->{vmstatestorage}", ['Datastore.AllocateSpace'])
> @@ -164,6 +209,87 @@ my $check_storage_access_migrate = sub {
>  	if !$scfg->{content}->{images};
>  };
>  
> +my $import_from_volid = sub {
> +    my ($storecfg, $src_volid, $dest_info, $vollist) = @_;
> +
> +    die "cannot import from cloudinit disk\n"
> +	if PVE::QemuServer::Drive::drive_is_cloudinit({ file => $src_volid });
> +
> +    my ($src_storeid, $src_volname) = PVE::Storage::parse_volume_id($src_volid);

technically this is already implied by the sub's name, we checked it 
already outside, but we need the store id for the bwlimit below..

> +    my $src_vmid = (PVE::Storage::parse_volname($storecfg, $src_volid))[2];
> +
> +    my $src_vm_state = sub {
> +	my $exists = $src_vmid && PVE::Cluster::get_vmlist()->{ids}->{$src_vmid} ? 1 : 0;
> +
> +	my $runs = 0;
> +	if ($exists) {
> +	    eval { PVE::QemuConfig::assert_config_exists_on_node($src_vmid); };
> +	    die "owner VM $src_vmid not on local node\n" if $@;
> +	    $runs = PVE::QemuServer::Helpers::vm_running_locally($src_vmid) || 0;
> +	}
> +
> +	return ($exists, $runs);
> +    };
> +
> +    my ($src_vm_exists, $running) = $src_vm_state->();
> +
> +    die "cannot import from '$src_volid' - full clone feature is not supported\n"
> +	if !PVE::Storage::volume_has_feature($storecfg, 'copy', $src_volid, undef, $running);
> +
> +    my $clonefn = sub {
> +	my ($src_vm_exists_now, $running_now) = $src_vm_state->();
> +
> +	die "owner VM $src_vmid changed state unexpectedly\n"
> +	    if $src_vm_exists_now != $src_vm_exists || $running_now != $running;
> +
> +	my $src_conf = $src_vm_exists_now ? PVE::QemuConfig->load_config($src_vmid) : {};
> +
> +	my $src_drive = { file => $src_volid };
> +	my $src_drivename;
> +	PVE::QemuConfig->foreach_volume($src_conf, sub {
> +	    my ($ds, $drive) = @_;
> +
> +	    return if $src_drivename;
> +
> +	    if ($drive->{file} eq $src_volid) {
> +		$src_drive = $drive;
> +		$src_drivename = $ds;
> +	    }
> +	});
> +
> +	my $source_info = {
> +	    vmid => $src_vmid,
> +	    running => $running_now,
> +	    drivename => $src_drivename,
> +	    drive => $src_drive,
> +	    snapname => undef,
> +	};
> +
> +	return PVE::QemuServer::clone_disk(
> +	    $storecfg,
> +	    $source_info,
> +	    $dest_info,
> +	    1,
> +	    $vollist,
> +	    undef,
> +	    undef,
> +	    $src_conf->{agent},
> +	    PVE::Storage::get_bandwidth_limit('clone', [$src_storeid, $dest_info->{storage}]),
> +	);
> +    };
> +
> +    my $cloned;
> +    if ($running) {
> +	$cloned = PVE::QemuConfig->lock_config_full($src_vmid, 30, $clonefn);
> +    } elsif ($src_vmid) {
> +	$cloned = PVE::QemuConfig->lock_config_shared($src_vmid, 30, $clonefn);
> +    } else {
> +	$cloned = $clonefn->();
> +    }
> +
> +    return $cloned->@{qw(file size)};
> +};
> +
>  # Note: $pool is only needed when creating a VM, because pool permissions
>  # are automatically inherited if VM already exists inside a pool.
>  my $create_disks = sub {
> @@ -207,28 +333,75 @@ my $create_disks = sub {
>  	} elsif ($volid =~ $NEW_DISK_RE) {
>  	    my ($storeid, $size) = ($2 || $default_storage, $3);
>  	    die "no storage ID specified (and no default storage)\n" if !$storeid;
> -	    my $defformat = PVE::Storage::storage_default_format($storecfg, $storeid);
> -	    my $fmt = $disk->{format} || $defformat;
> -
> -	    $size = PVE::Tools::convert_size($size, 'gb' => 'kb'); # vdisk_alloc uses kb
> -
> -	    my $volid;
> -	    if ($ds eq 'efidisk0') {
> -		my $smm = PVE::QemuServer::Machine::machine_type_is_q35($conf);
> -		($volid, $size) = PVE::QemuServer::create_efidisk(
> -		    $storecfg, $storeid, $vmid, $fmt, $arch, $disk, $smm);
> -	    } elsif ($ds eq 'tpmstate0') {
> -		# swtpm can only use raw volumes, and uses a fixed size
> -		$size = PVE::Tools::convert_size(PVE::QemuServer::Drive::TPMSTATE_DISK_SIZE, 'b' => 'kb');
> -		$volid = PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, "raw", undef, $size);
> +
> +	    if (my $source = delete $disk->{'import-from'}) {
> +		my $dst_volid;
> +		my ($src_storeid) = PVE::Storage::parse_volume_id($source, 1);
> +
> +		if ($src_storeid) { # PVE-managed volume

same as above applies here as well, $src_storeid is not used here, so 
can be shortened.

> +		    die "could not get size of $source\n"
> +			if !PVE::Storage::volume_size_info($storecfg, $source, 10);

this could move into $import_from_volid?

> +
> +		    my $dest_info = {
> +			vmid => $vmid,
> +			drivename => $ds,
> +			storage => $storeid,
> +			format => $disk->{format},
> +		    };
> +
> +		    $dest_info->{efisize} = PVE::QemuServer::get_efivars_size($conf, $disk)
> +			if $ds eq 'efidisk0';
> +
> +		    ($dst_volid, $size) = eval {
> +			$import_from_volid->($storecfg, $source, $dest_info, $vollist);
> +		    };
> +		    die "cannot import from '$source' - $@" if $@;
> +		} else {
> +		    $source = PVE::Storage::abs_filesystem_path($storecfg, $source, 1);
> +		    $size = PVE::Storage::file_size_info($source);
> +		    die "could not get file size of $source\n" if !$size;
> +
> +		    (undef, $dst_volid) = PVE::QemuServer::ImportDisk::do_import(
> +			$source,
> +			$vmid,
> +			$storeid,
> +			{
> +			    drive_name => $ds,
> +			    format => $disk->{format},
> +			    'skip-config-update' => 1,
> +			},
> +		    );
> +		    push @$vollist, $dst_volid;
> +		}
> +
> +		$disk->{file} = $dst_volid;
> +		$disk->{size} = $size;
> +		delete $disk->{format}; # no longer needed
> +		$res->{$ds} = PVE::QemuServer::print_drive($disk);
>  	    } else {
> -		$volid = PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, $fmt, undef, $size);
> +		my $defformat = PVE::Storage::storage_default_format($storecfg, $storeid);
> +		my $fmt = $disk->{format} || $defformat;
> +
> +		$size = PVE::Tools::convert_size($size, 'gb' => 'kb'); # vdisk_alloc uses kb
> +
> +		my $volid;
> +		if ($ds eq 'efidisk0') {
> +		    my $smm = PVE::QemuServer::Machine::machine_type_is_q35($conf);
> +		    ($volid, $size) = PVE::QemuServer::create_efidisk(
> +			$storecfg, $storeid, $vmid, $fmt, $arch, $disk, $smm);
> +		} elsif ($ds eq 'tpmstate0') {
> +		    # swtpm can only use raw volumes, and uses a fixed size
> +		    $size = PVE::Tools::convert_size(PVE::QemuServer::Drive::TPMSTATE_DISK_SIZE, 'b' => 'kb');
> +		    $volid = PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, "raw", undef, $size);
> +		} else {
> +		    $volid = PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, $fmt, undef, $size);
> +		}
> +		push @$vollist, $volid;
> +		$disk->{file} = $volid;
> +		$disk->{size} = PVE::Tools::convert_size($size, 'kb' => 'b');
> +		delete $disk->{format}; # no longer needed
> +		$res->{$ds} = PVE::QemuServer::print_drive($disk);
>  	    }
> -	    push @$vollist, $volid;
> -	    $disk->{file} = $volid;
> -	    $disk->{size} = PVE::Tools::convert_size($size, 'kb' => 'b');
> -	    delete $disk->{format}; # no longer needed
> -	    $res->{$ds} = PVE::QemuServer::print_drive($disk);
>  	} else {
>  	    PVE::Storage::check_volume_access($rpcenv, $authuser, $storecfg, $vmid, $volid);
>  
> @@ -242,7 +415,7 @@ my $create_disks = sub {
>  	}
>      };
>  
> -    eval { PVE::QemuConfig->foreach_volume($settings, $code); };
> +    eval { $foreach_volume_with_alloc->($settings, $code); };
>  
>      # free allocated images on error
>      if (my $err = $@) {
> @@ -1285,7 +1458,7 @@ my $update_vm_api  = sub {
>  
>  	    my $check_drive_perms = sub {
>  		my ($opt, $val) = @_;
> -		my $drive = PVE::QemuServer::parse_drive($opt, $val);
> +		my $drive = PVE::QemuServer::parse_drive($opt, $val, 1);

same applies here (move to previous patch?)

>  		# FIXME: cloudinit: CDROM or Disk?
>  		if (PVE::QemuServer::drive_is_cdrom($drive)) { # CDROM
>  		    $rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.CDROM']);
> @@ -1391,7 +1564,7 @@ my $update_vm_api  = sub {
>  		    # default legacy boot order implies all cdroms anyway
>  		    if (@bootorder) {
>  			# append new CD drives to bootorder to mark them bootable
> -			my $drive = PVE::QemuServer::parse_drive($opt, $param->{$opt});
> +			my $drive = PVE::QemuServer::parse_drive($opt, $param->{$opt}, 1);

same

>  			if (PVE::QemuServer::drive_is_cdrom($drive, 1) && !grep(/^$opt$/, @bootorder)) {
>  			    push @bootorder, $opt;
>  			    $conf->{pending}->{boot} = PVE::QemuServer::print_bootorder(\@bootorder);
> diff --git a/PVE/QemuServer/Drive.pm b/PVE/QemuServer/Drive.pm
> index d5d4723..88f013a 100644
> --- a/PVE/QemuServer/Drive.pm
> +++ b/PVE/QemuServer/Drive.pm
> @@ -409,6 +409,22 @@ my $alldrive_fmt = {
>      %efitype_fmt,
>  };
>  
> +my %import_from_fmt = (
> +    'import-from' => {
> +	type => 'string',
> +	format => 'pve-volume-id-or-absolute-path',
> +	format_description => 'source volume',
> +	description => "Create a new disk, importing from this source. If the volume is not ".
> +	    "managed by Proxmox VE, it's up to you to ensure that it's not actively used by ".
> +	    "another process during the import!",
> +	optional => 1,
> +    },
> +);
> +my $alldrive_fmt_with_alloc = {
> +    %$alldrive_fmt,
> +    %import_from_fmt,
> +};
> +
>  my $unused_fmt = {
>      volume => { alias => 'file' },
>      file => {
> @@ -436,6 +452,8 @@ my $desc_with_alloc = sub {
>  
>      my $new_desc = dclone($desc);
>  
> +    $new_desc->{format}->{'import-from'} = $import_from_fmt{'import-from'};
> +
>      my $extra_note = '';
>      if ($type eq 'efidisk') {
>  	$extra_note = " Note that SIZE_IN_GiB is ignored here and that the default EFI vars are ".
> @@ -445,7 +463,8 @@ my $desc_with_alloc = sub {
>      }
>  
>      $new_desc->{description} .= " Use the special syntax STORAGE_ID:SIZE_IN_GiB to allocate a new ".
> -	"volume.${extra_note}";
> +	"volume.${extra_note} Use STORAGE_ID:0 and the 'import-from' parameter to import from an ".
> +	"existing volume.";
>  
>      $with_alloc_desc_cache->{$type} = $new_desc;
>  
> @@ -547,7 +566,7 @@ sub drive_is_read_only {
>  #        [,iothread=on][,serial=serial][,model=model]
>  
>  sub parse_drive {
> -    my ($key, $data) = @_;
> +    my ($key, $data, $with_alloc) = @_;

technically previous patch, same as all the other changes in this file 
below this change

>  
>      my ($interface, $index);
>  
> @@ -558,12 +577,14 @@ sub parse_drive {
>  	return;
>      }
>  
> -    if (!defined($drivedesc_hash->{$key})) {
> +    my $desc_hash = $with_alloc ? $drivedesc_hash_with_alloc : $drivedesc_hash;
> +
> +    if (!defined($desc_hash->{$key})) {
>  	warn "invalid drive key: $key\n";
>  	return;
>      }
>  
> -    my $desc = $drivedesc_hash->{$key}->{format};
> +    my $desc = $desc_hash->{$key}->{format};
>      my $res = eval { PVE::JSONSchema::parse_property_string($desc, $data) };
>      return if !$res;
>      $res->{interface} = $interface;
> @@ -623,9 +644,10 @@ sub parse_drive {
>  }
>  
>  sub print_drive {
> -    my ($drive) = @_;
> +    my ($drive, $with_alloc) = @_;
>      my $skip = [ 'index', 'interface' ];
> -    return PVE::JSONSchema::print_property_string($drive, $alldrive_fmt, $skip);
> +    my $fmt = $with_alloc ? $alldrive_fmt_with_alloc : $alldrive_fmt;
> +    return PVE::JSONSchema::print_property_string($drive, $fmt, $skip);
>  }
>  
>  sub get_bootdisks {
> diff --git a/PVE/QemuServer/ImportDisk.pm b/PVE/QemuServer/ImportDisk.pm
> index 51ad52e..7557cac 100755
> --- a/PVE/QemuServer/ImportDisk.pm
> +++ b/PVE/QemuServer/ImportDisk.pm
> @@ -71,7 +71,7 @@ sub do_import {
>  	PVE::Storage::activate_volumes($storecfg, [$dst_volid]);
>  	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);
> +	PVE::QemuConfig->lock_config($vmid, $create_drive) if !$params->{'skip-config-update'};

should probably be added to the comment at start, even if it has a 
speaking name ;) skiplock is missing as well.

>      };
>      if (my $err = $@) {
>  	eval { PVE::Storage::vdisk_free($storecfg, $dst_volid) };
> -- 
> 2.30.2
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 

Re: [pve-devel] [PATCH v12 qemu-server 15/16] api: support VM disk import

[Fabian Ebner]

Am 14.03.22 um 16:54 schrieb Fabian Grünbichler:
> On March 9, 2022 11:09 am, Fabian Ebner wrote:

---snip---

>>  my $check_drive_param = sub {
>>      my ($param, $storecfg, $extra_checks) = @_;
>>  
>>      for my $opt (sort keys $param->%*) {
>>  	next if !PVE::QemuServer::is_valid_drivename($opt);
>>  
>> -	my $drive = PVE::QemuServer::parse_drive($opt, $param->{$opt});
>> +	my $drive = PVE::QemuServer::parse_drive($opt, $param->{$opt}, 1);
> 
> technically belongs into the previous patch, our non-alloc schema is 
> just tolerant enough because it doesn't look at the volids too closely 
> and accepts the NEW_DISK_RE syntax as potential existing volid..
> 

Makes sense. I guess I wanted to keep the other patch minimal, but
there's no good reason for that.

>>  	raise_param_exc({ $opt => "unable to parse drive options" }) if !$drive;
>>  
>> +	if ($drive->{'import-from'}) {
>> +	    die "'import-from' requires special syntax - use <storage ID>:0,import-from=<source>\n"
>> +		if $drive->{file} !~ $NEW_DISK_RE || $3 != 0;
> 
> should probably be a param_exc
> 
>> +
>> +	    if ($opt eq 'efidisk0') {
>> +		for my $required (qw(efitype pre-enrolled-keys)) {
>> +		    die "$opt - need to specify '$required' when using 'import-from'\n"
>> +			if !defined($drive->{$required});
> 
> same here
> 
>> +		}
>> +	    } elsif ($opt eq 'tpmstate0') {
>> +		die "$opt - need to specify 'version' when using 'import-from'\n"
>> +		    if !defined($drive->{version});
> 
> and here
> 

Will change it.

>> +	    }
>> +	}
>> +
>>  	PVE::QemuServer::cleanup_drive_path($opt, $storecfg, $drive);
>>  
>>  	$extra_checks->($drive) if $extra_checks;
>>  
>> -	$param->{$opt} = PVE::QemuServer::print_drive($drive);
>> +	$param->{$opt} = PVE::QemuServer::print_drive($drive, 1);
>>      }
>>  };
>>  
>> -my $NEW_DISK_RE = qr!^(([^/:\s]+):)?(\d+(\.\d+)?)$!;
>>  my $check_storage_access = sub {
>>     my ($rpcenv, $authuser, $storecfg, $vmid, $settings, $default_storage) = @_;
>>  
>> -   PVE::QemuConfig->foreach_volume($settings, sub {
>> +   $foreach_volume_with_alloc->($settings, sub {
>>  	my ($ds, $drive) = @_;
>>  
>>  	my $isCDROM = PVE::QemuServer::drive_is_cdrom($drive);
>> @@ -106,6 +137,20 @@ my $check_storage_access = sub {
>>  	} else {
>>  	    PVE::Storage::check_volume_access($rpcenv, $authuser, $storecfg, $vmid, $volid);
>>  	}
>> +
>> +	if (my $src_image = $drive->{'import-from'}) {
>> +	    my $src_vmid;
>> +	    my ($src_storeid) = PVE::Storage::parse_volume_id($src_image, 1);
>> +	    if ($src_storeid) { # PVE-managed volume
> 
> nit, could be
> 
> if (PVE::Storage::parse_volume_id($src_image, 1)) { # PVE-managed
> 
> since we don't actually care about the sid here, and parse_volume_id 
> will return undef when $noerr is set.
> 
>> +		$src_vmid = (PVE::Storage::parse_volname($storecfg, $src_image))[2]
> 
> is there some case where we expect parse_volume_id to work, but the 
> volume to not have an associated guest? because perl doesn't mind us 
> accessing the resulting array at arbitrary indices, so this doesn't fail 
> if $src_vmid is undef..
> 

Yes, when importing from an iscsi storage (not sure if there's other
cases). The check below and $import_from_volid both handle the case
where $src_vmid is undef.

> these should probably also check some more stuff (at least the volume 
> type?) - else we get strange errors when attempting to import 
> non-image-volumes (some of which even have owners, for example backup 
> archives..), and what exactly gets caught where is basically up to the 
> storage plugin via parse_volname and volume_has_feature..

Will add a check for vtype.

> 
>> +	    }
>> +
>> +	    if ($src_vmid) { # might be actively used by VM and will be copied via clone_disk()
>> +		$rpcenv->check($authuser, "/vms/${src_vmid}", ['VM.Clone']);
>> +	    } else {
>> +		PVE::Storage::check_volume_access($rpcenv, $authuser, $storecfg, $vmid, $src_image);
>> +	    }
>> +	}
>>      });
>>  
>>     $rpcenv->check($authuser, "/storage/$settings->{vmstatestorage}", ['Datastore.AllocateSpace'])
>> @@ -164,6 +209,87 @@ my $check_storage_access_migrate = sub {
>>  	if !$scfg->{content}->{images};
>>  };
>>  
>> +my $import_from_volid = sub {
>> +    my ($storecfg, $src_volid, $dest_info, $vollist) = @_;
>> +
>> +    die "cannot import from cloudinit disk\n"
>> +	if PVE::QemuServer::Drive::drive_is_cloudinit({ file => $src_volid });
>> +
>> +    my ($src_storeid, $src_volname) = PVE::Storage::parse_volume_id($src_volid);
> 
> technically this is already implied by the sub's name, we checked it 
> already outside, but we need the store id for the bwlimit below..
> 

Yes, it's not intended to be a check, although if it does fail something
went terribly wrong and it's good that we abort ;) I'll move it closer
to where it's actually used and I'll drop the unused $src_volname.

---snip---

>> +	    if (my $source = delete $disk->{'import-from'}) {
>> +		my $dst_volid;
>> +		my ($src_storeid) = PVE::Storage::parse_volume_id($source, 1);
>> +
>> +		if ($src_storeid) { # PVE-managed volume
> 
> same as above applies here as well, $src_storeid is not used here, so 
> can be shortened.
> 
>> +		    die "could not get size of $source\n"
>> +			if !PVE::Storage::volume_size_info($storecfg, $source, 10);
> 
> this could move into $import_from_volid?

Will do.

---snip---

>> @@ -242,7 +415,7 @@ my $create_disks = sub {
>>  	}
>>      };
>>  
>> -    eval { PVE::QemuConfig->foreach_volume($settings, $code); };
>> +    eval { $foreach_volume_with_alloc->($settings, $code); };
>>  
>>      # free allocated images on error
>>      if (my $err = $@) {
>> @@ -1285,7 +1458,7 @@ my $update_vm_api  = sub {
>>  
>>  	    my $check_drive_perms = sub {
>>  		my ($opt, $val) = @_;
>> -		my $drive = PVE::QemuServer::parse_drive($opt, $val);
>> +		my $drive = PVE::QemuServer::parse_drive($opt, $val, 1);
> 
> same applies here (move to previous patch?)
> 
>>  		# FIXME: cloudinit: CDROM or Disk?
>>  		if (PVE::QemuServer::drive_is_cdrom($drive)) { # CDROM
>>  		    $rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.CDROM']);
>> @@ -1391,7 +1564,7 @@ my $update_vm_api  = sub {
>>  		    # default legacy boot order implies all cdroms anyway
>>  		    if (@bootorder) {
>>  			# append new CD drives to bootorder to mark them bootable
>> -			my $drive = PVE::QemuServer::parse_drive($opt, $param->{$opt});
>> +			my $drive = PVE::QemuServer::parse_drive($opt, $param->{$opt}, 1);
> 
> same
> 

---snip---

>> @@ -547,7 +566,7 @@ sub drive_is_read_only {
>>  #        [,iothread=on][,serial=serial][,model=model]
>>  
>>  sub parse_drive {
>> -    my ($key, $data) = @_;
>> +    my ($key, $data, $with_alloc) = @_;
> 
> technically previous patch, same as all the other changes in this file 
> below this change
> 

Ack.

[pve-devel] [PATCH v12 qemu-server 16/16] api: update vm: print drive string for newly allocated/imported drives

[Fabian Ebner]

In the spirit of c75bf16 ("qm importdisk: tell user to what VM disk we
actually imported"), and so that the information is not lost once qm
importdisk switches to re-using the API call.

Added for cloudinit too, because a new disk is allocated.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

The name for cloudinit is rather predictable, so not too sure if it's
worth it there.

 PVE/API2/Qemu.pm | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 9220ce2..c64b92c 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -330,6 +330,7 @@ my $create_disks = sub {
 	    push @$vollist, $volid;
 	    delete $disk->{format}; # no longer needed
 	    $res->{$ds} = PVE::QemuServer::print_drive($disk);
+	    print "$ds: successfully created disk '$res->{$ds}'\n";
 	} elsif ($volid =~ $NEW_DISK_RE) {
 	    my ($storeid, $size) = ($2 || $default_storage, $3);
 	    die "no storage ID specified (and no default storage)\n" if !$storeid;
@@ -402,6 +403,8 @@ my $create_disks = sub {
 		delete $disk->{format}; # no longer needed
 		$res->{$ds} = PVE::QemuServer::print_drive($disk);
 	    }
+
+	    print "$ds: successfully created disk '$res->{$ds}'\n";
 	} else {
 	    PVE::Storage::check_volume_access($rpcenv, $authuser, $storecfg, $vmid, $volid);
 
-- 
2.30.2

[pve-devel] [PATCH v12 manager 1/1] api: nodes: add readovf endpoint

[Fabian Ebner]

Because the paths under /nodes/{node}/qemu/ are already occupied by
a {vmid} regex, it's not possible to use /nodes/{node}/qemu/readovf
for the new call. As the call does not depend upon a particular vmid,
it's placed under /nodes/{node} instead.

Signed-off-by: Dominic Jäger <d.jaeger@proxmox.com>
[split into its own patch + add to index]
Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

Needs dependency bump for qemu-server.

 PVE/API2/Nodes.pm | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/PVE/API2/Nodes.pm b/PVE/API2/Nodes.pm
index 655493a3..f595808a 100644
--- a/PVE/API2/Nodes.pm
+++ b/PVE/API2/Nodes.pm
@@ -49,6 +49,7 @@ use PVE::API2::LXC;
 use PVE::API2::Network;
 use PVE::API2::NodeConfig;
 use PVE::API2::Qemu::CPU;
+use PVE::API2::Qemu::OVF;
 use PVE::API2::Qemu;
 use PVE::API2::Replication;
 use PVE::API2::Services;
@@ -71,6 +72,11 @@ __PACKAGE__->register_method ({
     path => 'qemu',
 });
 
+__PACKAGE__->register_method ({
+    subclass => "PVE::API2::Qemu::OVF",
+    path => 'readovf',
+});
+
 __PACKAGE__->register_method ({
     subclass => "PVE::API2::LXC",
     path => 'lxc',
@@ -233,6 +239,7 @@ __PACKAGE__->register_method ({
 	    { name => 'network' },
 	    { name => 'qemu' },
 	    { name => 'query-url-metadata' },
+	    { name => 'readovf' },
 	    { name => 'replication' },
 	    { name => 'report' },
 	    { name => 'rrd' }, # fixme: remove?
-- 
2.30.2

[pve-devel] partially-applied: [PATCH-SERIES v12 qemu-server/manager] API for disk import and OVF

[Fabian Grünbichler]

applied qemu-server patches except 11 and 14-16, see comments on 
indivudal patches.

some unrelated but possibly fix-able as followup things I noticed:
- cloning a running VM with an EFI disk fails, the EFI disk is not 
  mirrorable (so we need another check like for TPM state?)
- cancelling a running clone doesn't cleanup properly (stops with trying 
  to aquire lock and leaves the target VM locked & existing)


On March 9, 2022 11:09 am, Fabian Ebner wrote:
> Extend qm importdisk/importovf functionality to the API.
> 
> Changes from v11 (after an off-list discussion with Fabian G.):
>     * Avoid confusing $dest->{conf} parameter for clone_disk().
>     * Require relevant parameters to be set explicitly for EFI/TPM.
>     * Base calculation of EFI vars size on passed-in parameters.
> 
> Changes from v10:
>     * Add fix for device unplug issue (patch #1).
>     * Add fixes related to calling create_disks() (patches #2 #3).
>     * Refactor clone_disk() in preparation to re-use it for import
>       (patches #4 #5 #6).
>     * Add patch to print the newly allocated drive (patch #14).
>     * Switch to using clone_disk for PVE-managed volumes and check for
>       VM.Clone in the permission check if there is an owner ID.
>     * Require <storeid>:0 syntax when using import-from. Allowing
>       other values than 0 for the size would be confusing, because
>       with import-from that size is never used (the size of the source
>       image is).
>     * Avoid making all foreach_volume iterators parse with the
>       extended schema. Instead, provide a custom iterator for the
>       places where it's actually required.
> 
> Still missing GUI integration for import from ovf, but that will be it's
> own series.
> 
> Older discussion:
> https://lists.proxmox.com/pipermail/pve-devel/2022-January/051379.html
> 
> 
> qemu-server:
> 
> Dominic Jäger (1):
>   api: support VM disk import
> 
> Fabian Ebner (15):
>   device unplug: verify that unplugging scsi disk completed
>   api: create disks: always activate/update size when attaching existing
>     volume
>   api: update: pass correct config when creating disks
>   clone disk: remove check for min QEMU version 2.7
>   clone disk: group source and target parameters
>   clone disk: pass in efi vars size rather than config
>   clone disk: allow cloning from an unused or unreferenced disk
>   efivars size: allow overriding efidisk parameter
>   schema: add pve-volume-id-or-absolute-path
>   parse ovf: untaint path when calling file_size_info
>   api: add endpoint for parsing .ovf files
>   image convert: allow block device as source
>   api: factor out check/cleanup for drive params
>   schema: drive: use separate schema when disk allocation is possible
>   api: update vm: print drive string for newly allocated/imported drives
> 
>  PVE/API2/Qemu.pm             | 381 +++++++++++++++++++++++++++--------
>  PVE/API2/Qemu/Makefile       |   2 +-
>  PVE/API2/Qemu/OVF.pm         |  55 +++++
>  PVE/QemuServer.pm            | 106 +++++++---
>  PVE/QemuServer/Drive.pm      |  94 ++++++---
>  PVE/QemuServer/ImportDisk.pm |   2 +-
>  PVE/QemuServer/OVF.pm        |   9 +-
>  7 files changed, 508 insertions(+), 141 deletions(-)
>  create mode 100644 PVE/API2/Qemu/OVF.pm
> 
> 
> manager:
> 
> Fabian Ebner (1):
>   api: nodes: add readovf endpoint
> 
>  PVE/API2/Nodes.pm | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> -- 
> 2.30.2
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 

Re: [pve-devel] partially-applied: [PATCH-SERIES v12 qemu-server/manager] API for disk import and OVF

[Fabian Ebner]

Am 14.03.22 um 16:57 schrieb Fabian Grünbichler:
> applied qemu-server patches except 11 and 14-16, see comments on 
> indivudal patches.
> 

Thanks a lot for the review/feedback!

> some unrelated but possibly fix-able as followup things I noticed:
> - cloning a running VM with an EFI disk fails, the EFI disk is not 
>   mirrorable (so we need another check like for TPM state?)

Isn't that just when the target storage allocates a different-sized
disk, i.e. https://bugzilla.proxmox.com/show_bug.cgi?id=3227

> - cancelling a running clone doesn't cleanup properly (stops with trying 
>   to aquire lock and leaves the target VM locked & existing)
> 

Will take a look.

Re: [pve-devel] partially-applied: [PATCH-SERIES v12 qemu-server/manager] API for disk import and OVF

[Fabian Grünbichler]

On March 16, 2022 11:00 am, Fabian Ebner wrote:
> Am 14.03.22 um 16:57 schrieb Fabian Grünbichler:
>> applied qemu-server patches except 11 and 14-16, see comments on 
>> indivudal patches.
>> 
> 
> Thanks a lot for the review/feedback!
> 
>> some unrelated but possibly fix-able as followup things I noticed:
>> - cloning a running VM with an EFI disk fails, the EFI disk is not 
>>   mirrorable (so we need another check like for TPM state?)
> 
> Isn't that just when the target storage allocates a different-sized
> disk, i.e. https://bugzilla.proxmox.com/show_bug.cgi?id=3227

no, was my fault (the VM in question had an efi disk, but was not booted 
using UEFI). probably should add a check for that as well though, 
unrelated to this series (move disk is also affected, and I guess 
live-migration as well..)

>> - cancelling a running clone doesn't cleanup properly (stops with trying 
>>   to aquire lock and leaves the target VM locked & existing)
>> 
> 
> Will take a look.
> 

the exact log messages are not always the same, but the target remains 
around locked with whatever state it managed to get to (and the task 
stopped with 'unexpected status').

Re: [pve-devel] partially-applied: [PATCH-SERIES v12 qemu-server/manager] API for disk import and OVF

[Fabian Ebner]

Am 16.03.22 um 11:29 schrieb Fabian Grünbichler:
> On March 16, 2022 11:00 am, Fabian Ebner wrote:
>> Am 14.03.22 um 16:57 schrieb Fabian Grünbichler:
>>> applied qemu-server patches except 11 and 14-16, see comments on 
>>> indivudal patches.
>>>
>>
>> Thanks a lot for the review/feedback!
>>
>>> some unrelated but possibly fix-able as followup things I noticed:
>>> - cloning a running VM with an EFI disk fails, the EFI disk is not 
>>>   mirrorable (so we need another check like for TPM state?)
>>
>> Isn't that just when the target storage allocates a different-sized
>> disk, i.e. https://bugzilla.proxmox.com/show_bug.cgi?id=3227
> 
> no, was my fault (the VM in question had an efi disk, but was not booted 
> using UEFI). probably should add a check for that as well though, 
> unrelated to this series (move disk is also affected, and I guess 
> live-migration as well..)

Would it be enough to have a prominent warning when starting the VM,
because it's already a configuration issue there.

> 
>>> - cancelling a running clone doesn't cleanup properly (stops with trying 
>>>   to aquire lock and leaves the target VM locked & existing)
>>>
>>
>> Will take a look.
>>
> 
> the exact log messages are not always the same, but the target remains 
> around locked with whatever state it managed to get to (and the task 
> stopped with 'unexpected status').

For me, this seems specific to RBD? And only when stopping via killing
the task via API/GUI which kills after 5 seconds. When interrupting on
the CLI, it hangs for a while but eventually cleans up.

Re: [pve-devel] partially-applied: [PATCH-SERIES v12 qemu-server/manager] API for disk import and OVF

[Fabian Grünbichler]

On March 16, 2022 12:25 pm, Fabian Ebner wrote:
> Am 16.03.22 um 11:29 schrieb Fabian Grünbichler:
>> On March 16, 2022 11:00 am, Fabian Ebner wrote:
>>> Am 14.03.22 um 16:57 schrieb Fabian Grünbichler:
>>>> applied qemu-server patches except 11 and 14-16, see comments on 
>>>> indivudal patches.
>>>>
>>>
>>> Thanks a lot for the review/feedback!
>>>
>>>> some unrelated but possibly fix-able as followup things I noticed:
>>>> - cloning a running VM with an EFI disk fails, the EFI disk is not 
>>>>   mirrorable (so we need another check like for TPM state?)
>>>
>>> Isn't that just when the target storage allocates a different-sized
>>> disk, i.e. https://bugzilla.proxmox.com/show_bug.cgi?id=3227
>> 
>> no, was my fault (the VM in question had an efi disk, but was not booted 
>> using UEFI). probably should add a check for that as well though, 
>> unrelated to this series (move disk is also affected, and I guess 
>> live-migration as well..)
> 
> Would it be enough to have a prominent warning when starting the VM,
> because it's already a configuration issue there.

yeah, a warning at startup and maybe marking the EFI disk on the GUI 
somehow - we do have both relevant settings available there?

I think warnings at startup are easily missed, but checking for such 
invalid configs in all operations that might possibly get called also 
seems like overkill (and the error if it happens is somewhat speaking 
anyway - it says there is no drive node named 'drive-efidisk0' ;))

>>>> - cancelling a running clone doesn't cleanup properly (stops with trying 
>>>>   to aquire lock and leaves the target VM locked & existing)
>>>>
>>>
>>> Will take a look.
>>>
>> 
>> the exact log messages are not always the same, but the target remains 
>> around locked with whatever state it managed to get to (and the task 
>> stopped with 'unexpected status').
> 
> For me, this seems specific to RBD? And only when stopping via killing
> the task via API/GUI which kills after 5 seconds. When interrupting on
> the CLI, it hangs for a while but eventually cleans up.

ZFS here (and yeah, clicking the stop button on the GUI), haven't tried 
other storages.. if there are some easy wins for improving this I'd go 
for it, but like I said, not related to this series at all, just 
something I noticed while testing.. flows where regular users can easily 
create config-locked guests are kinda cumbersome though.