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 EF06E95065 for ; Thu, 11 Apr 2024 18:49:48 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id CC113352BC for ; Thu, 11 Apr 2024 18:49:48 +0200 (CEST) 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)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Thu, 11 Apr 2024 18:49:48 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id EA91D44C01 for ; Thu, 11 Apr 2024 18:49:47 +0200 (CEST) Message-ID: <1f933fd5-f53d-4945-ace6-28c0bac63937@proxmox.com> Date: Thu, 11 Apr 2024 18:49:45 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Beta To: Proxmox VE development discussion , Dominik Csapak References: <20240410110401.2226201-1-d.csapak@proxmox.com> <20240410110401.2226201-3-d.csapak@proxmox.com> Content-Language: en-GB From: Thomas Lamprecht In-Reply-To: <20240410110401.2226201-3-d.csapak@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL -0.057 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 URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [pci.pm] Subject: Re: [pve-devel] [PATCH guest-common v2 2/5] mapping: pci: rework properties check 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: Thu, 11 Apr 2024 16:49:49 -0000 On 10/04/2024 13:03, Dominik Csapak wrote: > refactors the actual checking out to its own sub, so we can reuse it > later > > Signed-off-by: Dominik Csapak > --- > src/PVE/Mapping/PCI.pm | 43 +++++++++++++++++++++++++----------------- > 1 file changed, 26 insertions(+), 17 deletions(-) > > diff --git a/src/PVE/Mapping/PCI.pm b/src/PVE/Mapping/PCI.pm > index 725e106..fcf07c0 100644 > --- a/src/PVE/Mapping/PCI.pm > +++ b/src/PVE/Mapping/PCI.pm > @@ -129,6 +129,26 @@ sub options { > }; > } > > +my sub check_properties { s/check/assert/ and ideally some words that better describe what is actually asserted here. > + my ($correct, $configured, $path, $name) = @_; maybe s/correct/expected/ would be slightly better in conveying that the passed $configured one do not only need to be all in the first hash, but that all keys of the first hash > + for my $prop (sort keys %$correct) { > + next if !defined($correct->{$prop}) && !defined($configured->{$prop}); > + > + die "no '$prop' for device '$path'\n" pre-existing, but maybe this would be slightly better worded like: "missing expected property '$prop' for device '$path'\n" (no hard feelings though) > + if defined($correct->{$prop}) && !defined($configured->{$prop}); > + die "'$prop' configured but should not be\n" also pre-existing, but I would adapt the error message to something like: "unknown property '$prop' configured for device '$path'\n" (slightly hard feelings here ;-)) (btw. would it make sense to also add $name?) > + if !defined($correct->{$prop}) && defined($configured->{$prop}); can above check even trigger if we just go through the expected ($correct) set of properties? Or are existing, but undef, entries in $correct the forbidden ones, and other extra properties in $configured do not matter? (I dind't check the full picture, so excuse me if this would be obvious, but them IMO some comments would be warranted) > + > + my $correct_prop = $correct->{$prop}; > + $correct_prop =~ s/0x//g; > + my $configured_prop = $configured->{$prop}; > + $configured_prop =~ s/0x//g; > + > + die "'$prop' does not match for '$name' ($correct_prop != $configured_prop)\n" > + if $correct_prop ne $configured_prop; > + } > +} > + > # checks if the given config is valid for the current node > sub assert_valid { > my ($name, $cfg) = @_; > @@ -150,30 +170,19 @@ sub assert_valid { > > my $correct_props = { > id => "$info->{vendor}:$info->{device}", > - iommugroup => $info->{iommugroup}, > }; > > + # check iommu only on the first device > + if ($idx == 0) { > + $correct_props->{iommugroup} = $info->{iommugroup}; > + } is this really the same than what got removed in the loop? As if the next ID > + > if (defined($info->{'subsystem_vendor'}) && defined($info->{'subsystem_device'})) { > $correct_props->{'subsystem-id'} = "$info->{'subsystem_vendor'}:$info->{'subsystem_device'}"; > } > > - for my $prop (sort keys %$correct_props) { > - next if $prop eq 'iommugroup' && $idx > 0; # check iommu only on the first device > - > - next if !defined($correct_props->{$prop}) && !defined($cfg->{$prop}); > - die "no '$prop' for device '$path'\n" > - if defined($correct_props->{$prop}) && !defined($cfg->{$prop}); > - die "'$prop' configured but should not be\n" > - if !defined($correct_props->{$prop}) && defined($cfg->{$prop}); > + check_properties($correct_props, $cfg, $path, $name); > > - my $correct_prop = $correct_props->{$prop}; > - $correct_prop =~ s/0x//g; > - my $configured_prop = $cfg->{$prop}; > - $configured_prop =~ s/0x//g; > - > - die "'$prop' does not match for '$name' ($correct_prop != $configured_prop)\n" > - if $correct_prop ne $configured_prop; > - } > $idx++; > } >