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

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

I mean, the properties are relatively straight forward, but some commit
message would be still nice to have – e.g., how did you check if the values
propagate into the guest, can this

On 25/10/2023 14:37, Hannes Duerr wrote:
> Signed-off-by: Hannes Duerr <h.duerr@proxmox.com>
> ---
>  PVE/QemuServer.pm       | 12 ++++++++++++
>  PVE/QemuServer/Drive.pm | 26 ++++++++++++++++++++++++++
>  2 files changed, 38 insertions(+)
> 
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index 2cd8948..69be3af 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -1482,6 +1482,18 @@ sub print_drivedevice_full {
>  	}
>  	$device .= ",wwn=$drive->{wwn}" if $drive->{wwn};
>  
> +	# only scsi-hd supports passing vendor and product information

should we error out then if it's set to other types? Not here, as it's
already in the config, but erroring out on on config update/create could
be better UX than accepting it, but then not using it.

> +	if ($devicetype eq 'hd') {
> +	    if (my $vendor = $drive->{vendor}) {
> +		$vendor = URI::Escape::uri_unescape($vendor);
> +		$device .= ",vendor=$vendor";
> +	    }
> +	    if (my $product = $drive->{product}) {
> +		$product = URI::Escape::uri_unescape($product);
> +		$device .= ",product=$product";
> +	    }
> +	}
> +
>      } elsif ($drive->{interface} eq 'ide' || $drive->{interface} eq 'sata') {
>  	my $maxdev = ($drive->{interface} eq 'sata') ? $PVE::QemuServer::Drive::MAX_SATA_DISKS : 2;
>  	my $controller = int($drive->{index} / $maxdev);
> diff --git a/PVE/QemuServer/Drive.pm b/PVE/QemuServer/Drive.pm
> index e24ba12..20efc2f 100644
> --- a/PVE/QemuServer/Drive.pm
> +++ b/PVE/QemuServer/Drive.pm
> @@ -159,6 +159,28 @@ my %iothread_fmt = ( iothread => {
>  	optional => 1,
>  });
>  
> +my %product_fmt = (
> +    product => {
> +	type => 'string',
> +	format => 'urlencoded',
> +	format_description => 'product',
> +	maxLength => 40*3, # *3 since it's %xx url enoded

wouldn't that be for the worst case, e.g., if one would only enter spaces
or colons? And what about utf-8, that would be even bigger (not sure though
of we support that here).  

> +	description => "The drive's product name, url-encoded, up to 40 bytes long.",

the 40 bytesa aren't checked though? We would need to do so manually
after decoding it.

> +	optional => 1,
> +    },
> +);
> +
> +my %vendor_fmt = (
> +    vendor => {
> +	type => 'string',
> +	format => 'urlencoded',
> +	format_description => 'vendor',
> +	maxLength => 40*3, # *3 since it's %xx url enoded
> +	description => "The drive's vendor name, url-encoded, up to 40 bytes long.",

same here w.r.t. maxLength and 40 bytes max as above.

> +	optional => 1,
> +    },
> +);
> +
>  my %model_fmt = (
>      model => {
>  	type => 'string',