* [pve-devel] [PATCH qemu-server] fix #4957: add vendor and product information passthrough for SCSI-Disks
@ 2023-10-25 12:37 Hannes Duerr
2023-10-30 16:30 ` Thomas Lamprecht
0 siblings, 1 reply; 3+ messages in thread
From: Hannes Duerr @ 2023-10-25 12:37 UTC (permalink / raw)
To: pve-devel
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
+ 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
+ description => "The drive's product name, url-encoded, up to 40 bytes long.",
+ 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.",
+ optional => 1,
+ },
+);
+
my %model_fmt = (
model => {
type => 'string',
@@ -281,6 +303,8 @@ my $scsi_fmt = {
%scsiblock_fmt,
%ssd_fmt,
%wwn_fmt,
+ %vendor_fmt,
+ %product_fmt,
};
my $scsidesc = {
optional => 1,
@@ -404,6 +428,8 @@ my $alldrive_fmt = {
%readonly_fmt,
%scsiblock_fmt,
%ssd_fmt,
+ %vendor_fmt,
+ %product_fmt,
%wwn_fmt,
%tpmversion_fmt,
%efitype_fmt,
--
2.39.2
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [pve-devel] [PATCH qemu-server] fix #4957: add vendor and product information passthrough for SCSI-Disks
2023-10-25 12:37 [pve-devel] [PATCH qemu-server] fix #4957: add vendor and product information passthrough for SCSI-Disks Hannes Duerr
@ 2023-10-30 16:30 ` Thomas Lamprecht
2023-10-31 11:04 ` Wolfgang Bumiller
0 siblings, 1 reply; 3+ messages in thread
From: Thomas Lamprecht @ 2023-10-30 16:30 UTC (permalink / raw)
To: Proxmox VE development discussion, Hannes Duerr
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',
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [pve-devel] [PATCH qemu-server] fix #4957: add vendor and product information passthrough for SCSI-Disks
2023-10-30 16:30 ` Thomas Lamprecht
@ 2023-10-31 11:04 ` Wolfgang Bumiller
0 siblings, 0 replies; 3+ messages in thread
From: Wolfgang Bumiller @ 2023-10-31 11:04 UTC (permalink / raw)
To: Thomas Lamprecht; +Cc: Proxmox VE development discussion, Hannes Duerr
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).
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-10-31 11:05 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-10-25 12:37 [pve-devel] [PATCH qemu-server] fix #4957: add vendor and product information passthrough for SCSI-Disks Hannes Duerr
2023-10-30 16:30 ` Thomas Lamprecht
2023-10-31 11:04 ` Wolfgang Bumiller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox