From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 63F0067541 for ; Thu, 30 Jul 2020 12:38:36 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 56C62163E9 for ; Thu, 30 Jul 2020 12:38:36 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [212.186.127.180]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id A0B75163DF for ; Thu, 30 Jul 2020 12:38:35 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 6330C433E7 for ; Thu, 30 Jul 2020 12:38:35 +0200 (CEST) Date: Thu, 30 Jul 2020 12:38:33 +0200 From: Dominic =?iso-8859-1?Q?J=E4ger?= To: pve-devel@lists.proxmox.com Message-ID: <20200730103833.GB511631@mala.proxmox.com> References: <20200707100412.81731-1-d.jaeger@proxmox.com> <20200707100412.81731-4-d.jaeger@proxmox.com> <863926e1-a312-93fb-9183-1c753667af34@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <863926e1-a312-93fb-9183-1c753667af34@proxmox.com> User-Agent: Mutt/1.10.1 (2018-07-13) X-SPAM-LEVEL: Spam detection results: 0 AWL 1.783 Adjusted score from AWL reputation of From: address KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pve-devel] [PATCH qemu-server 3/3] Move importdisk from qm to API X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 30 Jul 2020 10:38:36 -0000 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.