public inbox for pve-devel@lists.proxmox.com
 help / color / mirror / Atom feed
From: Wolfgang Bumiller <w.bumiller@proxmox.com>
To: Thomas Lamprecht <t.lamprecht@proxmox.com>
Cc: Proxmox VE development discussion <pve-devel@lists.proxmox.com>,
	 Hannes Duerr <h.duerr@proxmox.com>
Subject: Re: [pve-devel] [PATCH qemu-server] fix #4957: add vendor and product information passthrough for SCSI-Disks
Date: Tue, 31 Oct 2023 12:04:47 +0100	[thread overview]
Message-ID: <7656jxzfcvf7khoiyhcrnqjuzzoquq2kgulsvv3dkx2ex6nkbe@qsreiptl4ork> (raw)
In-Reply-To: <c0893733-091e-4bb4-9d43-a47b3c17c840@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).




      reply	other threads:[~2023-10-31 11:05 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-10-25 12:37 Hannes Duerr
2023-10-30 16:30 ` Thomas Lamprecht
2023-10-31 11:04   ` Wolfgang Bumiller [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=7656jxzfcvf7khoiyhcrnqjuzzoquq2kgulsvv3dkx2ex6nkbe@qsreiptl4ork \
    --to=w.bumiller@proxmox.com \
    --cc=h.duerr@proxmox.com \
    --cc=pve-devel@lists.proxmox.com \
    --cc=t.lamprecht@proxmox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal