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) server-digest SHA256) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 70A2DA03FA for ; Wed, 8 Nov 2023 15:28:56 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 57B9B8F19 for ; Wed, 8 Nov 2023 15:28:56 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Wed, 8 Nov 2023 15:28:55 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 2084F47421 for ; Wed, 8 Nov 2023 15:28:55 +0100 (CET) Message-ID: Date: Wed, 8 Nov 2023 15:28:53 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Content-Language: en-US To: Fiona Ebner , Proxmox VE development discussion References: <20231108085128.38933-1-h.duerr@proxmox.com> From: =?UTF-8?Q?Hannes_D=C3=BCrr?= In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.004 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record T_SCC_BODY_TEXT_LINE -0.01 - URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [qemuserver.pm, drive.pm, qemu.pm] Subject: Re: [pve-devel] [PATCH v2 qemu-server] fix #4957: add vendor and product information passthrough for SCSI-Disks 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: Wed, 08 Nov 2023 14:28:56 -0000 On 11/8/23 11:04, Fiona Ebner wrote: > Am 08.11.23 um 09:51 schrieb Hannes Duerr: >> adds vendor and product information for SCSI devices to the json schema and >> checks in the VM create/update API call if it is possible to add these to QEMU as a device option >> >> Signed-off-by: Hannes Duerr >> --- >> >> changes in v2: >> - when calling the API to create/update a VM, check whether the devices >> are "scsi-hd" or "scsi-cd" devices,where there is the option to add >> vendor and product information, if not error out >> - change the format in product_fmt and vendor_fmt to a pattern that only >> allows 40 characters consisting of upper and lower case letters, numbers and '-' and '_'. >> >> PVE/API2/Qemu.pm | 9 +++++ >> PVE/QemuServer.pm | 83 +++++++++++++++++++++++++++++------------ >> PVE/QemuServer/Drive.pm | 24 ++++++++++++ >> 3 files changed, 92 insertions(+), 24 deletions(-) >> >> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm >> index 38bdaab..6898ec9 100644 >> --- a/PVE/API2/Qemu.pm >> +++ b/PVE/API2/Qemu.pm >> @@ -1030,6 +1030,11 @@ __PACKAGE__->register_method({ >> ); >> $conf->{$_} = $created_opts->{$_} for keys $created_opts->%*; >> >> + foreach my $opt (keys $created_opts->%*) { > Style nit: new code should use for instead of foreach > > Maybe sort the keys to have a deterministic order. > >> + if ($opt =~ m/scsi/) { >> + PVE::QemuServer::check_scsi_feature_compatibility($opt, $created_opts, $conf, $storecfg, $param); > > Style nit: line too long (100 characters is our limit) > > Note that $created_opts was already merged into $conf two lines above. > I'd rather not introduce new usage of that variable. > > Can we do the check before creating the drive instead? We know if it's a > CD or pass-through and the path or if it's iscsi ahead of time and that > should be enough for the check, or what am I missing? I don't think its possible to check in advance as the config can still contain a not properly formed path like: 'local-lvm:5', which will be formed to the real path when creating the disk or am I mistaken ? >> + } >> + } >> if (!$conf->{boot}) { >> my $devs = PVE::QemuServer::get_default_bootdevices($conf); >> $conf->{boot} = PVE::QemuServer::print_bootorder($devs); >> @@ -1840,6 +1845,10 @@ my $update_vm_api = sub { >> ); >> $conf->{pending}->{$_} = $created_opts->{$_} for keys $created_opts->%*; >> >> + if ($opt =~ m/scsi/) { >> + PVE::QemuServer::check_scsi_feature_compatibility($opt, $created_opts, $conf, $storecfg, $param); >> + } >> + >> # default legacy boot order implies all cdroms anyway >> if (@bootorder) { >> # append new CD drives to bootorder to mark them bootable >> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm >> index dbcd568..919728b 100644 >> --- a/PVE/QemuServer.pm >> +++ b/PVE/QemuServer.pm >> @@ -26,6 +26,7 @@ use Storable qw(dclone); >> use Time::HiRes qw(gettimeofday usleep); >> use URI::Escape; >> use UUID; >> +use Data::Dumper; >> > Left-over from debugging ;)? You can also use > use JSON; > print to_json($whatever, { canonical => 1, pretty => 1 }); > which is often nicer IMHO thanks for the hint, I'll have a look 🙂 >> use PVE::Cluster qw(cfs_register_file cfs_read_file cfs_write_file); >> use PVE::CGroup; >> @@ -1428,6 +1429,53 @@ my sub get_drive_id { >> return "$drive->{interface}$drive->{index}"; >> } >> > Can you please split the patch in two? A preparatory ones for creating > this helper and another one for the feature. > > I also think the helper would be nicer in the QemuServer/Drive.pm > module, but would need more preparation first to avoid a cyclic > dependency. That is: > * change the function to take $machine_version instead of $machine_type > * move the path_is_scsi and the scsi_inquiry function called by that > also to the drive module. > >> +sub get_scsi_devicetype { >> + my ($drive, $storecfg, $machine_type) = @_; > It's unfortunate that this needs both $storecfg and $machine_type just > for that compatibility check for the rather old machine version. But I > guess there's not much we can do at the moment. > >> + >> + my $devicetype = 'hd'; >> + my $path = ''; >> + if (drive_is_cdrom($drive)) { >> + $devicetype = 'cd'; >> + } else { >> + if ($drive->{file} =~ m|^/|) { >> + $path = $drive->{file}; >> + if (my $info = path_is_scsi($path)) { >> + if ($info->{type} == 0 && $drive->{scsiblock}) { >> + $devicetype = 'block'; >> + } elsif ($info->{type} == 1) { # tape >> + $devicetype = 'generic'; >> + } >> + } >> + } else { >> + $path = PVE::Storage::path($storecfg, $drive->{file}); >> + } >> + >> + # for compatibility only, we prefer scsi-hd (#2408, #2355, #2380) >> + my $version = kvm_user_version(); >> + $version = extract_version($machine_type, $version); > Why was this changed during the move? It's one line more now. Also, this > can be changed > >> + if ($path =~ m/^iscsi\:\/\// && >> + !min_version($version, 4, 1)) { >> + $devicetype = 'generic'; >> + } >> + } >> + >> + return $devicetype; >> +} >> + >> +sub check_scsi_feature_compatibility { > This is rather an assert than a check, because it will die > >> + my($opt, $created_opts, $conf, $storecfg, $param) = @_; > Style nit: missing space after my > > This is a rather "heavy" signature. Can we get away with less? I.e. just > pass in the new drive (string), $storecfg and stuff for machine_type. > >> + >> + my $drive = parse_drive($opt, $created_opts->{$opt}); >> + my $machine_type = get_vm_machine($conf, undef, $conf->{arch}); >> + my $drivetype = get_scsi_devicetype($drive, $storecfg, $machine_type); >> + >> + if ($drivetype ne 'hd' && $drivetype ne 'cd') { >> + if ($param->{$opt} =~ m/vendor/ || $param->{$opt} =~ m/product/) { >> + die "only 'scsi-hd' and 'scsi-cd' devices support passing vendor and product information\n"; >> + } >> + } >> +} >> + > (...) what does that mean ? >> @@ -281,6 +301,8 @@ my $scsi_fmt = { >> %scsiblock_fmt, >> %ssd_fmt, >> %wwn_fmt, >> + %vendor_fmt, >> + %product_fmt, > Nit: rest is ordered alphabetically, new ones not > >> }; >> my $scsidesc = { >> optional => 1, >> @@ -404,6 +426,8 @@ my $alldrive_fmt = { >> %readonly_fmt, >> %scsiblock_fmt, >> %ssd_fmt, >> + %vendor_fmt, >> + %product_fmt, >> %wwn_fmt, >> %tpmversion_fmt, >> %efitype_fmt, > Nit: rest is mostly ordered alphabetically, new ones make it worse