Re: [pve-devel] [PATCH v2 qemu-server] fix #4957: add vendor and product information passthrough for SCSI-Disks

[Fiona Ebner <f.ebner@proxmox.com>]

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 <h.duerr@proxmox.com>
> ---
> 
> 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