[pve-devel] [RFC qemu-server 1/2] Move importdisk from qm to API

[Dominic Jäger]

On Sat, Jun 06, 2020 at 07:56:20PM +0200, Thomas Lamprecht wrote:
> On 5/22/20 12:08 PM, Dominic Jäger wrote:
> > I'd appreciate a few hints for the importdisk GUI feature.
> > 
> > Yes or no to automatically adding the drive as active (not unused)?
> 
> Well, IMO makes sense but I'd add an parameter for either a config ID (scsi0, sata2,
> ...) which it is added if free, and/or the bus (controller) where we then select the
> next free slot if any.

Thank you! Makes sense. I'll add that and, of course, also the rest of the feedback.
> > +	The image format has to be supported by qemu-img(1).",
> 
> please use concatenated multi line strings for description, i.e.:
> 
> description => "Import an external disk image into a VM. "
>     ."The image format has to be supported by qemu-img(1).",
> 
> > +    permissions => {
> > +	description => "You need 'VM.Config.Disk' permissions on /vms/{vmid},
> > +	    and 'Datastore.AllocateSpace' permissions on the storage.",
> > +	check => [ 'and',
> > +		   ['perm', '/vms/{vmid}', [ 'VM.Config.Disk' ]],
> > +		   ['perm', '/storage/{storage}', [ 'Datastore.AllocateSpace' ]],
> > +	    ],
> > +    },
> > +    parameters => {
> > +	additionalProperties => 0,
> > +	properties => PVE::QemuServer::json_config_properties ({
> 
> hmm, doesn't this puts _all_ of our VM config properties in here?? Most probably
> do not make sense, only some disk/bus related ones, I'd say.

I'll have to take a closer look at that.

> > +	foreach my $opt (keys %$param) {
> > +	    die ("Die because multiple drives\n") if defined($driveX);
> 
> nit: no parenthesis in die
> 
> > +
> > +	my $message = $drive ? "to drive $drive->{'file'} on" : 'as unused drive to';
> > +	print "Importing disk '$source' " . $message . " VM $vmid ...\n";
> 
> FYI: double quoted strings allow $variable interpolation, so you do not need to
> concat this extra.
Don't know why I missed this...