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

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

On Mon, Oct 30, 2023 at 05:30:15PM +0100, Thomas Lamprecht wrote:
> 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.

AFAICT that's how it is for the other existing `format => 'urlencoded'`
options.
Our schema validation isn't smart enough for this currently.

We *could* add this sort of thing, though, if we wanted to?

The registered format verifiers could get the value's schema passed as
an additional parameter, then the `pve_verify_urlencoded()` sub in
JSONSchema.pm could check the decoded length against
`$schema->{maxLength} / 3` or (or a custom "extension"
`decodedLength => <int>`, but that's actually just superfluous in a
way... Unfortunately we can't put the lower number in 'maxLength' since
that's checked independently of the validation sub).