From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id E99539E438 for ; Tue, 31 Oct 2023 12:05:19 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id CA6851A51E for ; Tue, 31 Oct 2023 12:04:49 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Tue, 31 Oct 2023 12:04:49 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id D4E8742D78 for ; Tue, 31 Oct 2023 12:04:48 +0100 (CET) Date: Tue, 31 Oct 2023 12:04:47 +0100 From: Wolfgang Bumiller To: Thomas Lamprecht Cc: Proxmox VE development discussion , Hannes Duerr Message-ID: <7656jxzfcvf7khoiyhcrnqjuzzoquq2kgulsvv3dkx2ex6nkbe@qsreiptl4ork> References: <20231025123719.38036-1-h.duerr@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-SPAM-LEVEL: Spam detection results: 0 AWL 0.103 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record T_SCC_BODY_TEXT_LINE -0.01 - URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [drive.pm, jsonschema.pm, qemuserver.pm] Subject: Re: [pve-devel] [PATCH qemu-server] fix #4957: add vendor and product information passthrough for SCSI-Disks X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 31 Oct 2023 11:05:20 -0000 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 > > --- > > 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 => `, 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).