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

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

no in-depth review, but a meta style issue and some nits in line, the former
could be fixed up on applying, the nits are not really important in general.

Am 17/11/2023 um 13:17 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
> 

please keep commit messages below 70 characters per line, where
possible:
https://pve.proxmox.com/wiki/Developer_Documentation#Commits_and_Commit_Messages

> @@ -1011,6 +1038,13 @@ __PACKAGE__->register_method({
>  		my $conf = $param;
>  		my $arch = PVE::QemuServer::get_vm_arch($conf);
>  
> +		for my $opt (sort keys $param->%*) {
> +		    if ($opt =~ m/^scsi(\d)+$/) {

nit: unnecessary capture group, not costly here but we normally try to
either avoid them or mark them as non-capturing (tiny performance benefit),
i.e., one of:

$opt =~ m/^scsi\d+$/
$opt =~ m/^scsi(?:\d)+$/


and fwiw, this could be made shorter by either

- reversing the match and skip to next loop iteration early:
  next if $opt !~ m/^scsi(\d)+$/;

- use grep 

for $scsi_disk (grep { /^scsi\d+$/ } keys $param->%*) {
    # ...
}


> +			assert_scsi_feature_compatibility(
> +			    $opt, $conf, $storecfg, $param->{$opt});
> +		    }
> +		}
> +
>  		$conf->{meta} = PVE::QemuServer::new_meta_info_string();
>  
>  		my $vollist = [];
> @@ -1826,6 +1860,11 @@ my $update_vm_api  = sub {
>  		    PVE::QemuServer::vmconfig_register_unused_drive($storecfg, $vmid, $conf, PVE::QemuServer::parse_drive($opt, $conf->{pending}->{$opt}))
>  			if defined($conf->{pending}->{$opt});
>  
> +		    if ($opt =~ m/^scsi(\d)+$/) {

same as above w.r.t. caputre group

> +			assert_scsi_feature_compatibility(
> +			    $opt, $conf, $storecfg, $param->{$opt});
> +		    }
> +
>  		    my (undef, $created_opts) = $create_disks->(
>  			$rpcenv,
>  			$authuser,