[pve-devel] [PATCH v3 0/3] Close #2886: Add GUI for importdisk

[pve-devel] [PATCH v3 0/3] Close #2886: Add GUI for importdisk

[Dominic Jäger]

This series makes importing disks possible via GUI which is one step to make
migrating from other hypervisors easier.

proxmox-widget-toolkit: Dominic Jäger (1):
  Proxmox.window.Edit: Introduce separate submitUrl

qemu-server: Dominic Jäger (1):
      Move importdisk from qm to API

pve-manager: Dominic Jäger (1):
      Hardware View: Add GUI for importdisk

-- 
2.20.1

[pve-devel] [PATCH widget-toolkit v3 1/3] Proxmox.window.Edit: Introduce separate submitUrl

[Dominic Jäger]

Signed-off-by: Dominic Jäger <d.jaeger@proxmox.com>
---
v2->v3:
 - Use a more general approach in the shared code base instead of coding
   specifically for importdisk

 src/window/Edit.js | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/src/window/Edit.js b/src/window/Edit.js
index d7972b6..45b380a 100644
--- a/src/window/Edit.js
+++ b/src/window/Edit.js
@@ -128,7 +128,16 @@ Ext.define('Proxmox.window.Edit', {
 	    values.background_delay = me.backgroundDelay;
 	}
 
-	let url = me.url;
+	/*
+	 * Usually Proxmox.window.Edit windows read a guest configuration from
+	 * nodes/{node}/qemu/{vmid}/config and write changes (clicking the
+	 * submit button) to the same path.
+	 * Use submitUrl to change only the write path to something different.
+	 * Example: importdisk reuses the PVE.qemu.HDEdit window and reads from
+	 * nodes/{node}/qemu/{vmid}/config but needs to write to
+	 * nodes/{node}/qemu/{vmid}/importdisk
+	 */
+	let url = me.submitUrl || me.url;
 	if (me.method === 'DELETE') {
 	    url = url + "?" + Ext.Object.toQueryString(values);
 	    values = undefined;
-- 
2.20.1

Re: [pve-devel] [PATCH widget-toolkit v3 1/3] Proxmox.window.Edit: Introduce separate submitUrl

[Dominik Csapak]

Why is that necessary?

can't you just change url on HDEdit before submitting?

in HDEdit something like this:

---

if (changedmodetoimport) {
     me.url = 'new url'
}

---

On 8/25/20 11:24 AM, Dominic Jäger wrote:
> Signed-off-by: Dominic Jäger <d.jaeger@proxmox.com>
> ---
> v2->v3:
>   - Use a more general approach in the shared code base instead of coding
>     specifically for importdisk
> 
>   src/window/Edit.js | 11 ++++++++++-
>   1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/src/window/Edit.js b/src/window/Edit.js
> index d7972b6..45b380a 100644
> --- a/src/window/Edit.js
> +++ b/src/window/Edit.js
> @@ -128,7 +128,16 @@ Ext.define('Proxmox.window.Edit', {
>   	    values.background_delay = me.backgroundDelay;
>   	}
>   
> -	let url = me.url;
> +	/*
> +	 * Usually Proxmox.window.Edit windows read a guest configuration from
> +	 * nodes/{node}/qemu/{vmid}/config and write changes (clicking the
> +	 * submit button) to the same path.
> +	 * Use submitUrl to change only the write path to something different.
> +	 * Example: importdisk reuses the PVE.qemu.HDEdit window and reads from
> +	 * nodes/{node}/qemu/{vmid}/config but needs to write to
> +	 * nodes/{node}/qemu/{vmid}/importdisk
> +	 */
> +	let url = me.submitUrl || me.url;
>   	if (me.method === 'DELETE') {
>   	    url = url + "?" + Ext.Object.toQueryString(values);
>   	    values = undefined;
> 

[pve-devel] [PATCH qemu-server v3 2/3] Move importdisk from qm to API

[Dominic Jäger]

Additionally, add parameters that enable directly (avoiding the intermediate
step as unused disk) attaching the disk to a bus/device with all known disk
options.

Required to create a GUI for importdisk.

Signed-off-by: Dominic Jäger <d.jaeger@proxmox.com>
---
v2->v3: unchanged

 PVE/API2/Qemu.pm             | 113 ++++++++++++++++++++++++++++++++++-
 PVE/CLI/qm.pm                |  57 +-----------------
 PVE/QemuServer/Drive.pm      |  14 +++++
 PVE/QemuServer/ImportDisk.pm |   2 +-
 4 files changed, 127 insertions(+), 59 deletions(-)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 8da616a..66e630d 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -24,6 +24,7 @@ use PVE::QemuServer;
 use PVE::QemuServer::Drive;
 use PVE::QemuServer::CPUConfig;
 use PVE::QemuServer::Monitor qw(mon_cmd);
+use PVE::QemuServer::ImportDisk qw(do_import);
 use PVE::QemuMigrate;
 use PVE::RPCEnvironment;
 use PVE::AccessControl;
@@ -45,8 +46,6 @@ BEGIN {
     }
 }
 
-use Data::Dumper; # fixme: remove
-
 use base qw(PVE::RESTHandler);
 
 my $opt_force_description = "Force physical removal. Without this, we simple remove the disk from the config file and create an additional configuration entry called 'unused[n]', which contains the volume ID. Unlink of unused[n] always cause physical removal.";
@@ -4265,4 +4264,114 @@ __PACKAGE__->register_method({
 	return PVE::QemuServer::Cloudinit::dump_cloudinit_config($conf, $param->{vmid}, $param->{type});
     }});
 
+__PACKAGE__->register_method ({
+    name => 'importdisk',
+    path => '{vmid}/importdisk',
+    method => 'POST',
+    protected => 1, # for worker upid file
+    proxyto => 'node',
+    description => "Import an external disk image into a VM. The image format ".
+	"has to be supported by qemu-img.",
+    permissions => {
+	check => [ 'and',
+	    [ 'perm', '/storage/{storage}', ['Datastore.AllocateTemplate']],
+	    [ 'perm', '/storage/{storage}', ['Datastore.AllocateSpace']],
+	    [ 'perm', '/vms/{vmid}', ['VM.Allocate']],
+	],
+    },
+    parameters => {
+	additionalProperties => 0,
+	properties => {
+	    node => get_standard_option('pve-node'),
+	    vmid => get_standard_option('pve-vmid',
+		{completion => \&PVE::QemuServer::complete_vmid}),
+	    source => {
+		description => "Path to the disk image to import",
+		type => 'string',
+	    },
+	    device => {
+		type => 'string',
+		description => "Bus/Device type of the new disk ".
+		    "(e.g. 'ide0', 'scsi2'). Will add the image as unused disk ".
+		    "if omitted.",
+		enum => [PVE::QemuServer::Drive::valid_drive_names()],
+		optional => 1,
+	    },
+	    device_options => {
+		type => 'string',
+		format => 'drive_options',
+		description => "Options to set for the new disk ".
+		    "(e.g. 'discard=on,backup=0')",
+		optional => 1,
+	    },
+	    storage => get_standard_option('pve-storage-id', {
+		description => "The storage to which the image will be imported to.",
+		completion => \&PVE::QemuServer::complete_storage,
+	    }),
+	    format => {
+		type => 'string',
+		description => 'Target format.',
+		enum => [ 'raw', 'qcow2', 'vmdk' ],
+		optional => 1,
+	    },
+	    digest => get_standard_option('pve-config-digest'),
+	},
+    },
+    returns => { type => 'string'},
+    code => sub {
+	my ($params) = @_;
+	my $rpcenv = PVE::RPCEnvironment::get();
+	my $authuser = $rpcenv->get_user();
+
+	my $vmid = extract_param($params, 'vmid');
+	my $source = extract_param($params, 'source');
+	my $digest = extract_param($params, 'digest');
+	my $device_options = extract_param($params, 'device_options');
+	my $device = extract_param($params, 'device');
+	my $storage = extract_param($params, 'storage');
+	my $vm_conf = PVE::QemuConfig->load_config($vmid);
+
+	die "Could not import because source parameter is an empty string\n"
+	    if ($source eq "");
+	die "Could not import because source '$source' is not an absolute path\n"
+	    if $source !~ m!/!;
+	die "Could not import because source '$source' does not exist"
+	    if !-e $source;
+	die "VM $vmid config checksum missmatch (file change by other user?)\n"
+	    if $digest && $digest ne $vm_conf->{digest};
+	die "Could not import because device $device is already in use in ".
+	    "VM $vmid. Choose a different device!\n"
+	    if $device && $vm_conf->{$device};
+
+	my $worker = sub {
+	    my $message = $device ? "to $device on" : 'as unused disk to';
+	    print "Importing disk '$source' $message VM $vmid ...\n";
+	    my ($unused_disk, $volid) = eval {
+		PVE::QemuServer::ImportDisk::do_import(
+		    $source, $vmid, $storage,
+		    {format => extract_param($params, 'format')},
+		)
+	    };
+	    die "Importing disk failed: $@\n" if $@;
+
+	    if ($device) {
+		eval {
+		    $update_vm_api->({
+			vmid => $vmid,
+			$device => "$volid,$device_options",
+		    }, 1);
+		};
+		# Importing large disks takes a lot of time
+		# => don't remove disk automatically on option update error
+		die "Copying disk to $unused_disk succeeded but attaching it ".
+		    "as $device and setting its options to $device_options ".
+		    "failed: $@.\n Adjust them manually.\n" if $@;
+	    }
+	    print "Successfully imported disk '$source' as ".
+		($device ? $device : $unused_disk) . ": $volid'\n";
+	};
+	my $upid = $rpcenv->fork_worker('importdisk', $vmid, $authuser, $worker);
+	return $upid;
+    }});
+
 1;
diff --git a/PVE/CLI/qm.pm b/PVE/CLI/qm.pm
index 282fa86..4a304ce 100755
--- a/PVE/CLI/qm.pm
+++ b/PVE/CLI/qm.pm
@@ -445,61 +445,6 @@ __PACKAGE__->register_method ({
 	return undef;
     }});
 
-__PACKAGE__->register_method ({
-    name => 'importdisk',
-    path => 'importdisk',
-    method => 'POST',
-    description => "Import an external disk image as an unused disk in a VM. The
- image format has to be supported by qemu-img(1).",
-    parameters => {
-	additionalProperties => 0,
-	properties => {
-	    vmid => get_standard_option('pve-vmid', {completion => \&PVE::QemuServer::complete_vmid}),
-	    source => {
-		description => 'Path to the disk image to import',
-		type => 'string',
-		optional => 0,
-	    },
-            storage => get_standard_option('pve-storage-id', {
-		description => 'Target storage ID',
-		completion => \&PVE::QemuServer::complete_storage,
-		optional => 0,
-            }),
-	    format => {
-		type => 'string',
-		description => 'Target format',
-		enum => [ 'raw', 'qcow2', 'vmdk' ],
-		optional => 1,
-	    },
-	},
-    },
-    returns => { type => 'null'},
-    code => sub {
-	my ($param) = @_;
-
-	my $vmid = extract_param($param, 'vmid');
-	my $source = extract_param($param, 'source');
-	my $storeid = extract_param($param, 'storage');
-	my $format = extract_param($param, 'format');
-
-	my $vm_conf = PVE::QemuConfig->load_config($vmid);
-	PVE::QemuConfig->check_lock($vm_conf);
-	die "$source: non-existent or non-regular file\n" if (! -f $source);
-
-	my $storecfg = PVE::Storage::config();
-	PVE::Storage::storage_check_enabled($storecfg, $storeid);
-
-	my $target_storage_config =  PVE::Storage::storage_config($storecfg, $storeid);
-	die "storage $storeid does not support vm images\n"
-	    if !$target_storage_config->{content}->{images};
-
-	print "importing disk '$source' to VM $vmid ...\n";
-	my ($drive_id, $volid) = PVE::QemuServer::ImportDisk::do_import($source, $vmid, $storeid, { format => $format });
-	print "Successfully imported disk as '$drive_id:$volid'\n";
-
-	return undef;
-    }});
-
 __PACKAGE__->register_method ({
     name => 'terminal',
     path => 'terminal',
@@ -984,7 +929,7 @@ our $cmddef = {
 
     terminal => [ __PACKAGE__, 'terminal', ['vmid']],
 
-    importdisk => [ __PACKAGE__, 'importdisk', ['vmid', 'source', 'storage']],
+    importdisk => [ "PVE::API2::Qemu", 'importdisk', ['vmid', 'source', 'storage'], { node => $nodename }],
 
     importovf => [ __PACKAGE__, 'importovf', ['vmid', 'manifest', 'storage']],
 
diff --git a/PVE/QemuServer/Drive.pm b/PVE/QemuServer/Drive.pm
index 91c33f8..e7cb74c 100644
--- a/PVE/QemuServer/Drive.pm
+++ b/PVE/QemuServer/Drive.pm
@@ -201,6 +201,7 @@ my %wwn_fmt = (
     },
 );
 
+
 my $add_throttle_desc = sub {
     my ($key, $type, $what, $unit, $longunit, $minimum) = @_;
     my $d = {
@@ -308,6 +309,19 @@ my $alldrive_fmt = {
     %wwn_fmt,
 };
 
+my %optional_file_drivedesc_base = %drivedesc_base;
+$optional_file_drivedesc_base{file}{optional} = 1;
+my $drive_options_fmt = {
+    %optional_file_drivedesc_base,
+    %iothread_fmt,
+    %model_fmt,
+    %queues_fmt,
+    %scsiblock_fmt,
+    %ssd_fmt,
+    %wwn_fmt,
+};
+PVE::JSONSchema::register_format('drive_options', $drive_options_fmt);
+
 my $efidisk_fmt = {
     volume => { alias => 'file' },
     file => {
diff --git a/PVE/QemuServer/ImportDisk.pm b/PVE/QemuServer/ImportDisk.pm
index 51ad52e..d89cd4d 100755
--- a/PVE/QemuServer/ImportDisk.pm
+++ b/PVE/QemuServer/ImportDisk.pm
@@ -38,7 +38,7 @@ sub do_import {
 	}
 
 	if ($drive_name) {
-	    # should never happen as setting $drive_name is not exposed to public interface
+	    # Exposed in importdisk API only
 	    die "cowardly refusing to overwrite existing entry: $drive_name\n" if $vm_conf->{$drive_name};
 
 	    my $modified = {}; # record what $option we modify
-- 
2.20.1

Re: [pve-devel] [PATCH qemu-server v3 2/3] Move importdisk from qm to API

[Dominik Csapak]

comments inline

On 8/25/20 11:24 AM, Dominic Jäger wrote:
> Additionally, add parameters that enable directly (avoiding the intermediate
> step as unused disk) attaching the disk to a bus/device with all known disk
> options.
> 
> Required to create a GUI for importdisk.
> 
> Signed-off-by: Dominic Jäger <d.jaeger@proxmox.com>
> ---
> v2->v3: unchanged
> 
>   PVE/API2/Qemu.pm             | 113 ++++++++++++++++++++++++++++++++++-
>   PVE/CLI/qm.pm                |  57 +-----------------
>   PVE/QemuServer/Drive.pm      |  14 +++++
>   PVE/QemuServer/ImportDisk.pm |   2 +-
>   4 files changed, 127 insertions(+), 59 deletions(-)
> 
> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
> index 8da616a..66e630d 100644
> --- a/PVE/API2/Qemu.pm
> +++ b/PVE/API2/Qemu.pm
> @@ -24,6 +24,7 @@ use PVE::QemuServer;
>   use PVE::QemuServer::Drive;
>   use PVE::QemuServer::CPUConfig;
>   use PVE::QemuServer::Monitor qw(mon_cmd);
> +use PVE::QemuServer::ImportDisk qw(do_import);
>   use PVE::QemuMigrate;
>   use PVE::RPCEnvironment;
>   use PVE::AccessControl;
> @@ -45,8 +46,6 @@ BEGIN {
>       }
>   }
>   
> -use Data::Dumper; # fixme: remove
> -
>   use base qw(PVE::RESTHandler);
>   
>   my $opt_force_description = "Force physical removal. Without this, we simple remove the disk from the config file and create an additional configuration entry called 'unused[n]', which contains the volume ID. Unlink of unused[n] always cause physical removal.";
> @@ -4265,4 +4264,114 @@ __PACKAGE__->register_method({
>   	return PVE::QemuServer::Cloudinit::dump_cloudinit_config($conf, $param->{vmid}, $param->{type});
>       }});
>   
> +__PACKAGE__->register_method ({
> +    name => 'importdisk',
> +    path => '{vmid}/importdisk',
> +    method => 'POST',
> +    protected => 1, # for worker upid file
> +    proxyto => 'node',
> +    description => "Import an external disk image into a VM. The image format ".
> +	"has to be supported by qemu-img.",
> +    permissions => {
> +	check => [ 'and',
> +	    [ 'perm', '/storage/{storage}', ['Datastore.AllocateTemplate']],
> +	    [ 'perm', '/storage/{storage}', ['Datastore.AllocateSpace']],
> +	    [ 'perm', '/vms/{vmid}', ['VM.Allocate']],
> +	],

this is a big no-no

now everyone that has permissions to create a vm can import any
file they want from any path?

(e.g. another vm image, /dev/sda (!!), etc.)

this is basically circumventing our whole permission system...

for 'qm' this was okay since that was root@pam only, but if
we are in the api, we have to be careful what to import

in general, we have to do either:
* restrict the source path to a very small subset of possible
   paths, that are guaranteed to not be dangerous
   (/tmp/pve-import-$userid/ ?)

* or leave it as 'root@pam' only

> +    },
> +    parameters => {
> +	additionalProperties => 0,
> +	properties => {
> +	    node => get_standard_option('pve-node'),
> +	    vmid => get_standard_option('pve-vmid',
> +		{completion => \&PVE::QemuServer::complete_vmid}),
> +	    source => {
> +		description => "Path to the disk image to import",
> +		type => 'string',
> +	    },
> +	    device => {
> +		type => 'string',
> +		description => "Bus/Device type of the new disk ".
> +		    "(e.g. 'ide0', 'scsi2'). Will add the image as unused disk ".
> +		    "if omitted.",
> +		enum => [PVE::QemuServer::Drive::valid_drive_names()],
> +		optional => 1,
> +	    },
> +	    device_options => {
> +		type => 'string',
> +		format => 'drive_options',
> +		description => "Options to set for the new disk ".
> +		    "(e.g. 'discard=on,backup=0')",
> +		optional => 1,
> +	    },
> +	    storage => get_standard_option('pve-storage-id', {
> +		description => "The storage to which the image will be imported to.",
> +		completion => \&PVE::QemuServer::complete_storage,
> +	    }),
> +	    format => {
> +		type => 'string',
> +		description => 'Target format.',
> +		enum => [ 'raw', 'qcow2', 'vmdk' ],
> +		optional => 1,
> +	    },
> +	    digest => get_standard_option('pve-config-digest'),
> +	},
> +    },
> +    returns => { type => 'string'},
> +    code => sub {
> +	my ($params) = @_;
> +	my $rpcenv = PVE::RPCEnvironment::get();
> +	my $authuser = $rpcenv->get_user();
> +
> +	my $vmid = extract_param($params, 'vmid');
> +	my $source = extract_param($params, 'source');
> +	my $digest = extract_param($params, 'digest');
> +	my $device_options = extract_param($params, 'device_options');
> +	my $device = extract_param($params, 'device');
> +	my $storage = extract_param($params, 'storage');
> +	my $vm_conf = PVE::QemuConfig->load_config($vmid);
> +
> +	die "Could not import because source parameter is an empty string\n"
> +	    if ($source eq "");
> +	die "Could not import because source '$source' is not an absolute path\n"
> +	    if $source !~ m!/!;
> +	die "Could not import because source '$source' does not exist"
> +	    if !-e $source;
> +	die "VM $vmid config checksum missmatch (file change by other user?)\n"
> +	    if $digest && $digest ne $vm_conf->{digest};
> +	die "Could not import because device $device is already in use in ".
> +	    "VM $vmid. Choose a different device!\n"
> +	    if $device && $vm_conf->{$device};
> +
> +	my $worker = sub {
> +	    my $message = $device ? "to $device on" : 'as unused disk to';
> +	    print "Importing disk '$source' $message VM $vmid ...\n";
> +	    my ($unused_disk, $volid) = eval {
> +		PVE::QemuServer::ImportDisk::do_import(
> +		    $source, $vmid, $storage,
> +		    {format => extract_param($params, 'format')},
> +		)
> +	    };
> +	    die "Importing disk failed: $@\n" if $@;
> +
> +	    if ($device) {
> +		eval {
> +		    $update_vm_api->({
> +			vmid => $vmid,
> +			$device => "$volid,$device_options",
> +		    }, 1);
> +		};
> +		# Importing large disks takes a lot of time
> +		# => don't remove disk automatically on option update error
> +		die "Copying disk to $unused_disk succeeded but attaching it ".
> +		    "as $device and setting its options to $device_options ".
> +		    "failed: $@.\n Adjust them manually.\n" if $@;
> +	    }
> +	    print "Successfully imported disk '$source' as ".
> +		($device ? $device : $unused_disk) . ": $volid'\n";

is that not somehow racy?

i start an import and give devices scsi2

then at the right time i update the vm with scsi2 and now i overwrite it 
again?

should we not hold a lock ? or can we ignore that ? (at least document it?)

also if i see it right, 'do_import' can do this already?
you even change the comment of the code but you don't use it...

> +	};
> +	my $upid = $rpcenv->fork_worker('importdisk', $vmid, $authuser, $worker);
> +	return $upid;
> +    }});
> +
>   1;
> diff --git a/PVE/CLI/qm.pm b/PVE/CLI/qm.pm
> index 282fa86..4a304ce 100755
> --- a/PVE/CLI/qm.pm
> +++ b/PVE/CLI/qm.pm
> @@ -445,61 +445,6 @@ __PACKAGE__->register_method ({
>   	return undef;
>       }});
>   
> -__PACKAGE__->register_method ({
> -    name => 'importdisk',
> -    path => 'importdisk',
> -    method => 'POST',
> -    description => "Import an external disk image as an unused disk in a VM. The
> - image format has to be supported by qemu-img(1).",
> -    parameters => {
> -	additionalProperties => 0,
> -	properties => {
> -	    vmid => get_standard_option('pve-vmid', {completion => \&PVE::QemuServer::complete_vmid}),
> -	    source => {
> -		description => 'Path to the disk image to import',
> -		type => 'string',
> -		optional => 0,
> -	    },
> -            storage => get_standard_option('pve-storage-id', {
> -		description => 'Target storage ID',
> -		completion => \&PVE::QemuServer::complete_storage,
> -		optional => 0,
> -            }),
> -	    format => {
> -		type => 'string',
> -		description => 'Target format',
> -		enum => [ 'raw', 'qcow2', 'vmdk' ],
> -		optional => 1,
> -	    },
> -	},
> -    },
> -    returns => { type => 'null'},
> -    code => sub {
> -	my ($param) = @_;
> -
> -	my $vmid = extract_param($param, 'vmid');
> -	my $source = extract_param($param, 'source');
> -	my $storeid = extract_param($param, 'storage');
> -	my $format = extract_param($param, 'format');
> -
> -	my $vm_conf = PVE::QemuConfig->load_config($vmid);
> -	PVE::QemuConfig->check_lock($vm_conf);
> -	die "$source: non-existent or non-regular file\n" if (! -f $source);
> -
> -	my $storecfg = PVE::Storage::config();
> -	PVE::Storage::storage_check_enabled($storecfg, $storeid);
> -
> -	my $target_storage_config =  PVE::Storage::storage_config($storecfg, $storeid);
> -	die "storage $storeid does not support vm images\n"
> -	    if !$target_storage_config->{content}->{images};
> -
> -	print "importing disk '$source' to VM $vmid ...\n";
> -	my ($drive_id, $volid) = PVE::QemuServer::ImportDisk::do_import($source, $vmid, $storeid, { format => $format });
> -	print "Successfully imported disk as '$drive_id:$volid'\n";
> -
> -	return undef;
> -    }});
> -
>   __PACKAGE__->register_method ({
>       name => 'terminal',
>       path => 'terminal',
> @@ -984,7 +929,7 @@ our $cmddef = {
>   
>       terminal => [ __PACKAGE__, 'terminal', ['vmid']],
>   
> -    importdisk => [ __PACKAGE__, 'importdisk', ['vmid', 'source', 'storage']],
> +    importdisk => [ "PVE::API2::Qemu", 'importdisk', ['vmid', 'source', 'storage'], { node => $nodename }],
>   
>       importovf => [ __PACKAGE__, 'importovf', ['vmid', 'manifest', 'storage']],
>   
> diff --git a/PVE/QemuServer/Drive.pm b/PVE/QemuServer/Drive.pm
> index 91c33f8..e7cb74c 100644
> --- a/PVE/QemuServer/Drive.pm
> +++ b/PVE/QemuServer/Drive.pm
> @@ -201,6 +201,7 @@ my %wwn_fmt = (
>       },
>   );
>   
> +
>   my $add_throttle_desc = sub {
>       my ($key, $type, $what, $unit, $longunit, $minimum) = @_;
>       my $d = {
> @@ -308,6 +309,19 @@ my $alldrive_fmt = {
>       %wwn_fmt,
>   };
>   
> +my %optional_file_drivedesc_base = %drivedesc_base;
> +$optional_file_drivedesc_base{file}{optional} = 1;
> +my $drive_options_fmt = {
> +    %optional_file_drivedesc_base,
> +    %iothread_fmt,
> +    %model_fmt,
> +    %queues_fmt,
> +    %scsiblock_fmt,
> +    %ssd_fmt,
> +    %wwn_fmt,
> +};
> +PVE::JSONSchema::register_format('drive_options', $drive_options_fmt);
> +
>   my $efidisk_fmt = {
>       volume => { alias => 'file' },
>       file => {
> diff --git a/PVE/QemuServer/ImportDisk.pm b/PVE/QemuServer/ImportDisk.pm
> index 51ad52e..d89cd4d 100755
> --- a/PVE/QemuServer/ImportDisk.pm
> +++ b/PVE/QemuServer/ImportDisk.pm
> @@ -38,7 +38,7 @@ sub do_import {
>   	}
>   
>   	if ($drive_name) {
> -	    # should never happen as setting $drive_name is not exposed to public interface
> +	    # Exposed in importdisk API only
>   	    die "cowardly refusing to overwrite existing entry: $drive_name\n" if $vm_conf->{$drive_name};
>   
>   	    my $modified = {}; # record what $option we modify
> 

Re: [pve-devel] [PATCH qemu-server v3 2/3] Move importdisk from qm to API

[Thomas Lamprecht]

On 03.09.20 15:52, Dominik Csapak wrote:
> comments inline
> 
> On 8/25/20 11:24 AM, Dominic Jäger wrote:
>> Additionally, add parameters that enable directly (avoiding the intermediate
>> step as unused disk) attaching the disk to a bus/device with all known disk
>> options.
>>
>> Required to create a GUI for importdisk.
>>
>> Signed-off-by: Dominic Jäger <d.jaeger@proxmox.com>
>> ---
>> v2->v3: unchanged
>>
>>   PVE/API2/Qemu.pm             | 113 ++++++++++++++++++++++++++++++++++-
>>   PVE/CLI/qm.pm                |  57 +-----------------
>>   PVE/QemuServer/Drive.pm      |  14 +++++
>>   PVE/QemuServer/ImportDisk.pm |   2 +-
>>   4 files changed, 127 insertions(+), 59 deletions(-)
>>
>> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
>> index 8da616a..66e630d 100644
>> --- a/PVE/API2/Qemu.pm
>> +++ b/PVE/API2/Qemu.pm
>> @@ -24,6 +24,7 @@ use PVE::QemuServer;
>>   use PVE::QemuServer::Drive;
>>   use PVE::QemuServer::CPUConfig;
>>   use PVE::QemuServer::Monitor qw(mon_cmd);
>> +use PVE::QemuServer::ImportDisk qw(do_import);
>>   use PVE::QemuMigrate;
>>   use PVE::RPCEnvironment;
>>   use PVE::AccessControl;
>> @@ -45,8 +46,6 @@ BEGIN {
>>       }
>>   }
>>   -use Data::Dumper; # fixme: remove
>> -
>>   use base qw(PVE::RESTHandler);
>>     my $opt_force_description = "Force physical removal. Without this, we simple remove the disk from the config file and create an additional configuration entry called 'unused[n]', which contains the volume ID. Unlink of unused[n] always cause physical removal.";
>> @@ -4265,4 +4264,114 @@ __PACKAGE__->register_method({
>>       return PVE::QemuServer::Cloudinit::dump_cloudinit_config($conf, $param->{vmid}, $param->{type});
>>       }});
>>   +__PACKAGE__->register_method ({
>> +    name => 'importdisk',
>> +    path => '{vmid}/importdisk',
>> +    method => 'POST',
>> +    protected => 1, # for worker upid file
>> +    proxyto => 'node',
>> +    description => "Import an external disk image into a VM. The image format ".
>> +    "has to be supported by qemu-img.",
>> +    permissions => {
>> +    check => [ 'and',
>> +        [ 'perm', '/storage/{storage}', ['Datastore.AllocateTemplate']],
>> +        [ 'perm', '/storage/{storage}', ['Datastore.AllocateSpace']],
>> +        [ 'perm', '/vms/{vmid}', ['VM.Allocate']],
>> +    ],
> 
> this is a big no-no
> 
> now everyone that has permissions to create a vm can import any
> file they want from any path?
> 
> (e.g. another vm image, /dev/sda (!!), etc.)
> 
> this is basically circumventing our whole permission system...
> 
> for 'qm' this was okay since that was root@pam only, but if
> we are in the api, we have to be careful what to import
> 
> in general, we have to do either:
> * restrict the source path to a very small subset of possible
>   paths, that are guaranteed to not be dangerous
>   (/tmp/pve-import-$userid/ ?)

No, for sure not tmp, this can clash easily and is outside of every permission
management of us (PVE)

We already have paths which are owned and have permissions attached, allow
all paths the user can access anyway. This consists of all disk volumes/images
they can access already. We encode the owner (VMID) in the disk name after all
for a reason.

A disk image uploader, or "puller" (if it can pull it over https directly to
the server) should import such images with a defined name, ideally it gets 
already associated with a VMID and all of our ownership rules and enforcement
work out.

But yes, we do not want to allow using an arbitrary path.

[pve-devel] [PATCH manager v3 3/3] Hardware View: Add GUI for importdisk

[Dominic Jäger]

Make importing single disks easier.
Required to import a whole VM via GUI.

Signed-off-by: Dominic Jäger <d.jaeger@proxmox.com>
---
v2->v3: Use the new submitUrl parameter from widget-tookit

Depends on both other patches

 www/manager6/qemu/HDEdit.js       | 83 +++++++++++++++++++++++--------
 www/manager6/qemu/HardwareView.js | 20 ++++++++
 2 files changed, 81 insertions(+), 22 deletions(-)

diff --git a/www/manager6/qemu/HDEdit.js b/www/manager6/qemu/HDEdit.js
index e2a5b914..edebbbc1 100644
--- a/www/manager6/qemu/HDEdit.js
+++ b/www/manager6/qemu/HDEdit.js
@@ -76,23 +76,46 @@ Ext.define('PVE.qemu.HDInputPanel', {
 	    me.drive.format = values.diskformat;
 	}
 
-	PVE.Utils.propertyStringSet(me.drive, !values.backup, 'backup', '0');
-	PVE.Utils.propertyStringSet(me.drive, values.noreplicate, 'replicate', 'no');
-	PVE.Utils.propertyStringSet(me.drive, values.discard, 'discard', 'on');
-	PVE.Utils.propertyStringSet(me.drive, values.ssd, 'ssd', 'on');
-	PVE.Utils.propertyStringSet(me.drive, values.iothread, 'iothread', 'on');
-	PVE.Utils.propertyStringSet(me.drive, values.cache, 'cache');
-
-        var names = ['mbps_rd', 'mbps_wr', 'iops_rd', 'iops_wr'];
-        Ext.Array.each(names, function(name) {
-            var burst_name = name + '_max';
-	    PVE.Utils.propertyStringSet(me.drive, values[name], name);
-	    PVE.Utils.propertyStringSet(me.drive, values[burst_name], burst_name);
-        });
-
-
-	params[confid] = PVE.Parser.printQemuDrive(me.drive);
+	if (me.isImport) {
+	    // These keys & values are accepted by the API as they are
+	    let simple = ['backup', 'ssd', 'iothread', 'cache'];
+	    let burst = ['mbps_rd', 'mbps_wr', 'iops_rd', 'iops_wr'];
+	    burst = burst.concat(burst.map(x => `${x}_max`));
+	    let available = simple.concat(burst);
+
+	    let addValues = key => `${key}=${values[key]}`;
+	    let selectedKeys = x => values[x];
+	    let options = available.filter(selectedKeys).map(addValues).join();
+
+	    // These need modification for the API
+	    options += values.discard ? ',discard=on' : '';
+	    options += values.noreplicate ? ',replicate=0' : '';
+
+	    params.device_options = options;
+	} else {
+	    PVE.Utils.propertyStringSet(me.drive, !values.backup, 'backup', '0');
+	    PVE.Utils.propertyStringSet(me.drive, values.noreplicate, 'replicate', 'no');
+	    PVE.Utils.propertyStringSet(me.drive, values.discard, 'discard', 'on');
+	    PVE.Utils.propertyStringSet(me.drive, values.ssd, 'ssd', 'on');
+	    PVE.Utils.propertyStringSet(me.drive, values.iothread, 'iothread', 'on');
+	    PVE.Utils.propertyStringSet(me.drive, values.cache, 'cache');
+
+	    var names = ['mbps_rd', 'mbps_wr', 'iops_rd', 'iops_wr'];
+		Ext.Array.each(names, function(name) {
+		    var burst_name = name + '_max';
+		    PVE.Utils.propertyStringSet(me.drive, values[name], name);
+		    PVE.Utils.propertyStringSet(me.drive, values[burst_name], burst_name);
+	    });
+	}
 
+	if (me.isImport) {
+	    params.source = values.inputImage;
+	    params.device = values.controller + values.deviceid;
+	    params.storage = values.hdstorage;
+	    if (values.diskformat) params.format = values.diskformat;
+	} else {
+	    params[confid] = PVE.Parser.printQemuDrive(me.drive);
+	}
 	return params;
     },
 
@@ -199,14 +222,17 @@ Ext.define('PVE.qemu.HDInputPanel', {
 		allowBlank: false
 	    });
 	    me.column1.push(me.unusedDisks);
-	} else if (me.isCreate) {
-	    me.column1.push({
+	} else if (me.isCreate || me.isImport) {
+	    let selector = {
 		xtype: 'pveDiskStorageSelector',
 		storageContent: 'images',
 		name: 'disk',
 		nodename: me.nodename,
-		autoSelect: me.insideWizard
-	    });
+		hideSize: me.isImport,
+		autoSelect: me.insideWizard || me.isImport,
+	    };
+	    if (me.isImport) selector.storageLabel = gettext('Target storage');
+	    me.column1.push(selector);
 	} else {
 	    me.column1.push({
 		xtype: 'textfield',
@@ -231,6 +257,14 @@ Ext.define('PVE.qemu.HDInputPanel', {
 		name: 'discard'
 	    }
 	);
+	if (me.isImport) {
+	    me.column2.push({
+		xtype: 'textfield',
+		fieldLabel: gettext('Source image'),
+		name: 'inputImage',
+		emptyText: '/home/user/disk.qcow2',
+	    });
+	}
 
 	me.advancedColumn1.push(
 	    {
@@ -372,14 +406,19 @@ Ext.define('PVE.qemu.HDEdit', {
 	    confid: me.confid,
 	    nodename: nodename,
 	    unused: unused,
-	    isCreate: me.isCreate
+	    isCreate: me.isCreate,
+	    isImport: me.isImport,
 	});
 
 	var subject;
 	if (unused) {
 	    me.subject = gettext('Unused Disk');
+	} else if (me.isImport) {
+	    me.subject = gettext('Import Disk');
+	    me.submitText = 'Import';
+	    me.backgroundDelay = undefined;
 	} else if (me.isCreate) {
-            me.subject = gettext('Hard Disk');
+	    me.subject = gettext('Hard Disk');
 	} else {
            me.subject = gettext('Hard Disk') + ' (' + me.confid + ')';
 	}
diff --git a/www/manager6/qemu/HardwareView.js b/www/manager6/qemu/HardwareView.js
index 40b3fe86..5598214b 100644
--- a/www/manager6/qemu/HardwareView.js
+++ b/www/manager6/qemu/HardwareView.js
@@ -436,6 +436,25 @@ Ext.define('PVE.qemu.HardwareView', {
 	    handler: run_move
 	});
 
+	var import_btn = new Proxmox.button.Button({
+	    text: gettext('Import disk'),
+	    hidden: !(caps.vms['VM.Allocate'] &&
+		caps.storage['Datastore.AllocateTemplate'] &&
+		caps.storage['Datastore.AllocateSpace']),
+	    handler: function() {
+		let url = `/api2/extjs/${baseurl}`;
+		var win = Ext.create('PVE.qemu.HDEdit', {
+		    method: 'POST',
+		    url: url,
+		    submitUrl: url.replace('config', 'importdisk'),
+		    pveSelNode: me.pveSelNode,
+		    isImport: true,
+		});
+		win.on('destroy', me.reload, me);
+		win.show();
+	    },
+	});
+
 	var remove_btn = new Proxmox.button.Button({
 	    text: gettext('Remove'),
 	    defaultText: gettext('Remove'),
@@ -752,6 +771,7 @@ Ext.define('PVE.qemu.HardwareView', {
 		edit_btn,
 		resize_btn,
 		move_btn,
+		import_btn,
 		revert_btn
 	    ],
 	    rows: rows,
-- 
2.20.1

Re: [pve-devel] [PATCH manager v3 3/3] Hardware View: Add GUI for importdisk

[Dominik Csapak]

On 8/25/20 11:24 AM, Dominic Jäger wrote:
> Make importing single disks easier.
> Required to import a whole VM via GUI.
> 
> Signed-off-by: Dominic Jäger <d.jaeger@proxmox.com>
> ---
> v2->v3: Use the new submitUrl parameter from widget-tookit
> 
> Depends on both other patches
> 
>   www/manager6/qemu/HDEdit.js       | 83 +++++++++++++++++++++++--------
>   www/manager6/qemu/HardwareView.js | 20 ++++++++
>   2 files changed, 81 insertions(+), 22 deletions(-)
> 
> diff --git a/www/manager6/qemu/HDEdit.js b/www/manager6/qemu/HDEdit.js
> index e2a5b914..edebbbc1 100644
> --- a/www/manager6/qemu/HDEdit.js
> +++ b/www/manager6/qemu/HDEdit.js
> @@ -76,23 +76,46 @@ Ext.define('PVE.qemu.HDInputPanel', {
>   	    me.drive.format = values.diskformat;
>   	}
>   
> -	PVE.Utils.propertyStringSet(me.drive, !values.backup, 'backup', '0');
> -	PVE.Utils.propertyStringSet(me.drive, values.noreplicate, 'replicate', 'no');
> -	PVE.Utils.propertyStringSet(me.drive, values.discard, 'discard', 'on');
> -	PVE.Utils.propertyStringSet(me.drive, values.ssd, 'ssd', 'on');
> -	PVE.Utils.propertyStringSet(me.drive, values.iothread, 'iothread', 'on');
> -	PVE.Utils.propertyStringSet(me.drive, values.cache, 'cache');
> -
> -        var names = ['mbps_rd', 'mbps_wr', 'iops_rd', 'iops_wr'];
> -        Ext.Array.each(names, function(name) {
> -            var burst_name = name + '_max';
> -	    PVE.Utils.propertyStringSet(me.drive, values[name], name);
> -	    PVE.Utils.propertyStringSet(me.drive, values[burst_name], burst_name);
> -        });
> -
> -
> -	params[confid] = PVE.Parser.printQemuDrive(me.drive);
> +	if (me.isImport) {
> +	    // These keys & values are accepted by the API as they are
> +	    let simple = ['backup', 'ssd', 'iothread', 'cache'];
> +	    let burst = ['mbps_rd', 'mbps_wr', 'iops_rd', 'iops_wr'];
> +	    burst = burst.concat(burst.map(x => `${x}_max`));
> +	    let available = simple.concat(burst);
> +
> +	    let addValues = key => `${key}=${values[key]}`;
> +	    let selectedKeys = x => values[x];
> +	    let options = available.filter(selectedKeys).map(addValues).join();
> +
> +	    // These need modification for the API
> +	    options += values.discard ? ',discard=on' : '';
> +	    options += values.noreplicate ? ',replicate=0' : '';
> +
> +	    params.device_options = options;

why do you do this?

you can simply reuse the propertyStringSet function again, just
use a different object

...drive code...

let drive_obj = import ? {} : me.drive;

PVE.Utils.propertyStringSet(drive_obj....);

and then use drive_obj instead of either your values or me.drive
(you can use PVE.Parser.printPropertyString to get the 'propertyString' 
format)

on another note, here in onGetValues you can replace the url with the
correct submiturl if you want, no need to change widget toolkit

> +	} else {
> +	    PVE.Utils.propertyStringSet(me.drive, !values.backup, 'backup', '0');
> +	    PVE.Utils.propertyStringSet(me.drive, values.noreplicate, 'replicate', 'no');
> +	    PVE.Utils.propertyStringSet(me.drive, values.discard, 'discard', 'on');
> +	    PVE.Utils.propertyStringSet(me.drive, values.ssd, 'ssd', 'on');
> +	    PVE.Utils.propertyStringSet(me.drive, values.iothread, 'iothread', 'on');
> +	    PVE.Utils.propertyStringSet(me.drive, values.cache, 'cache');
> +
> +	    var names = ['mbps_rd', 'mbps_wr', 'iops_rd', 'iops_wr'];
> +		Ext.Array.each(names, function(name) {
> +		    var burst_name = name + '_max';
> +		    PVE.Utils.propertyStringSet(me.drive, values[name], name);
> +		    PVE.Utils.propertyStringSet(me.drive, values[burst_name], burst_name);
> +	    });
> +	}
>   
> +	if (me.isImport) {
> +	    params.source = values.inputImage;
> +	    params.device = values.controller + values.deviceid;
> +	    params.storage = values.hdstorage;
> +	    if (values.diskformat) params.format = values.diskformat;
> +	} else {
> +	    params[confid] = PVE.Parser.printQemuDrive(me.drive);
> +	}
>   	return params;
>       },
>   
> @@ -199,14 +222,17 @@ Ext.define('PVE.qemu.HDInputPanel', {
>   		allowBlank: false
>   	    });
>   	    me.column1.push(me.unusedDisks);
> -	} else if (me.isCreate) {
> -	    me.column1.push({
> +	} else if (me.isCreate || me.isImport) {
> +	    let selector = {
>   		xtype: 'pveDiskStorageSelector',
>   		storageContent: 'images',
>   		name: 'disk',
>   		nodename: me.nodename,
> -		autoSelect: me.insideWizard
> -	    });
> +		hideSize: me.isImport,
> +		autoSelect: me.insideWizard || me.isImport,
> +	    };
> +	    if (me.isImport) selector.storageLabel = gettext('Target storage');
> +	    me.column1.push(selector);
>   	} else {
>   	    me.column1.push({
>   		xtype: 'textfield',
> @@ -231,6 +257,14 @@ Ext.define('PVE.qemu.HDInputPanel', {
>   		name: 'discard'
>   	    }
>   	);
> +	if (me.isImport) {
> +	    me.column2.push({
> +		xtype: 'textfield',
> +		fieldLabel: gettext('Source image'),
> +		name: 'inputImage',
> +		emptyText: '/home/user/disk.qcow2',
> +	    });
> +	}
>   
>   	me.advancedColumn1.push(
>   	    {
> @@ -372,14 +406,19 @@ Ext.define('PVE.qemu.HDEdit', {
>   	    confid: me.confid,
>   	    nodename: nodename,
>   	    unused: unused,
> -	    isCreate: me.isCreate
> +	    isCreate: me.isCreate,
> +	    isImport: me.isImport,
>   	});
>   
>   	var subject;
>   	if (unused) {
>   	    me.subject = gettext('Unused Disk');
> +	} else if (me.isImport) {
> +	    me.subject = gettext('Import Disk');
> +	    me.submitText = 'Import';
> +	    me.backgroundDelay = undefined;
>   	} else if (me.isCreate) {
> -            me.subject = gettext('Hard Disk');
> +	    me.subject = gettext('Hard Disk');
>   	} else {
>              me.subject = gettext('Hard Disk') + ' (' + me.confid + ')';
>   	}
> diff --git a/www/manager6/qemu/HardwareView.js b/www/manager6/qemu/HardwareView.js
> index 40b3fe86..5598214b 100644
> --- a/www/manager6/qemu/HardwareView.js
> +++ b/www/manager6/qemu/HardwareView.js
> @@ -436,6 +436,25 @@ Ext.define('PVE.qemu.HardwareView', {
>   	    handler: run_move
>   	});
>   
> +	var import_btn = new Proxmox.button.Button({
> +	    text: gettext('Import disk'),
> +	    hidden: !(caps.vms['VM.Allocate'] &&
> +		caps.storage['Datastore.AllocateTemplate'] &&
> +		caps.storage['Datastore.AllocateSpace']),
> +	    handler: function() {
> +		let url = `/api2/extjs/${baseurl}`;
> +		var win = Ext.create('PVE.qemu.HDEdit', {
> +		    method: 'POST',
> +		    url: url,
> +		    submitUrl: url.replace('config', 'importdisk'),

this is wrong, replace only replaces the first instance

if my node is named 'confignode1'

the resulting url is now

/api2/extjs/nodes/importdisknode1/qemu/$ID/config

(which probably does no harm but will not work)

if you do this here, i would construct 2 baseurls
and construct both urls independendtly

if you decide to replace config with importdisk
in HDEdit, i would use a regex replace
  /\/config$/\/importdisk/
or use

url.lastIndexOf("/config")

to not run into such issues
> +		    pveSelNode: me.pveSelNode,
> +		    isImport: true,
> +		});
> +		win.on('destroy', me.reload, me);
> +		win.show();
> +	    },
> +	});
> +
>   	var remove_btn = new Proxmox.button.Button({
>   	    text: gettext('Remove'),
>   	    defaultText: gettext('Remove'),
> @@ -752,6 +771,7 @@ Ext.define('PVE.qemu.HardwareView', {
>   		edit_btn,
>   		resize_btn,
>   		move_btn,
> +		import_btn,
>   		revert_btn
>   	    ],
>   	    rows: rows,
>