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

["Dominic Jäger" <d.jaeger@proxmox.com>]

On Wed, Jul 29, 2020 at 03:00:41PM +0200, Aaron Lauterer wrote:
> Some small nits inline, mainly regarding the wording of descriptions and messages.
> 
> One thing I noticed is with multi line strings.
> The concat `.` should probably be at the end of the previous line if you split it up

I intially placed it in front because the Perl style guide has && in front in a
multi-line example. However, most of this file has dots in the end and the
guide forbids mixing only, so I changed it.

> but I would prefer to have long lines instead of it being split. Long lines
> make it easier to search for a certain (log) message in the code with grep.

Fabian G. just told me that 80 character line length limit also holds for comments.

> > +	properties => {
> > +	    node => get_standard_option('pve-node'),
> > +	    vmid => get_standard_option('pve-vmid',
> > +		{completion => \&PVE::QemuServer::complete_vmid}),
> > +	    source => {
> > +		description => 'Path of the disk image that should be imported',
> 
> What about: "Path to the disk image to import".
> 
> > +		type => 'string',
> > +	    },
> > +	    device => {
> > +		type => 'string',
> > +		description => "What device the imported image should be "
> 
> "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 => "What options to set for the device "
> 
> "Options to set for the new disk"
> 
> > +		    ."(e.g. 'discard=on,backup=0')",
> > +		optional => 1,
> > +	    },
> > +	    storage => get_standard_option('pve-storage-id', {
> > +		description => 'Which storage to use for the imported image.',
> 
> "The storage to which the image will be imported to."
Fits the rest of the descriptions better => Changed all of them. Thank you :)

> 
> > +	my $worker = sub {
> > +	    my $message = $device ? "to $device on" : 'as unused disk to';
> 
> wouldn't it make more sense to phrase it "to $device for" and 'as unused disk for'?

Didn't include this (yet) because I actually don't think so.
The idea was
1) Import sth to sth => "Importing disk 'myfile.qcow2' to VM100."
   How are we importing it? as unused disk
   "Importing disk 'myfile.qcow2' as unused disk to VM100."
2) Import sth to sth => "Importing disk 'myfile.qcow2' to scsi0."
   Where is scsi0? on VM100
   "Importing disk 'myfile.qcow2' to scsi0 on VM100."

> It will result in something like "importing disk 'myfile.qcow' as unused disk for VM XXX"
I think there should be a "to" somewhere.

Would for 2) "Importing disk 'myfile.qcow2' to VM100 as scsi0" make sense, maybe?
> 
> > +my %optional_file_drivdesc_base = %drivedesc_base;
> 
> s/drivdesc/drivedesc/ or maybe even to drive_desc
> 
> > +$optional_file_drivdesc_base{file}{optional} = 1; # TODO verify
> > +my $drive_options_fmt = {
> > +    %optional_file_drivdesc_base, # first bad thing here is the missing file stuff
> > +    %iothread_fmt,
> > +    %model_fmt,
> > +    %queues_fmt,
> > +    %scsiblock_fmt,
> > +    %ssd_fmt,
> > +    %wwn_fmt,
> > +};
> > +PVE::JSONSchema::register_format('drive_options', $drive_options_fmt);
> > +
Thank you, changed it to drivedesc :) It's drivedesc in the rest of the file, without underscore.