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).
prev parent 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