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 9AA7CA029C for ; Wed, 8 Nov 2023 11:05:03 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 78B995362 for ; Wed, 8 Nov 2023 11:04:33 +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 11:04:32 +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 8631D470F4 for ; Wed, 8 Nov 2023 11:04:32 +0100 (CET) Message-ID: Date: Wed, 8 Nov 2023 11:04:27 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: Proxmox VE development discussion , Hannes Duerr References: <20231108085128.38933-1-h.duerr@proxmox.com> Content-Language: en-US From: Fiona Ebner In-Reply-To: <20231108085128.38933-1-h.duerr@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL -0.080 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. [qemu.pm, drive.pm, qemuserver.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 10:05:03 -0000 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? > + } > + } > 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 > 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"; > + } > + } > +} > + (...) > @@ -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