From 72da25da974a1fa9d995b46afb76714d56f4cb9b Mon Sep 17 00:00:00 2001 Message-ID: <72da25da974a1fa9d995b46afb76714d56f4cb9b.1707394627.git.jdenemar@redhat.com> From: Peter Krempa Date: Tue, 30 Jan 2024 16:45:39 +0100 Subject: [PATCH] virPCIVPDParseVPDLargeResourceFields: Refactor return logic MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rewrite the conditions after exiting the parser so that they are easier to understand. This partially decreases the granularity of "error" messages as they are not strictly necessary albeit for debugging. As it was already observed in this code the logic itself often does something else than the comment claims, thus the code logic is preserved. Changes: - any case when not all data was processed is aggregated together and gets a common "error" message - absence of 'checksum' field is checked separately - helper variables are removed as they are no longer needed Signed-off-by: Peter Krempa Reviewed-by: Ján Tomko (cherry picked from commit a352bcf1c64e4efaa31ff5b193ff8ff4ca9d1329) https://issues.redhat.com/browse/RHEL-22314 [9.4.0] --- src/util/virpcivpd.c | 32 +++++++++++++------------------- 1 file changed, 13 insertions(+), 19 deletions(-) diff --git a/src/util/virpcivpd.c b/src/util/virpcivpd.c index 60e520c46f..be19f7b747 100644 --- a/src/util/virpcivpd.c +++ b/src/util/virpcivpd.c @@ -415,10 +415,7 @@ virPCIVPDParseVPDLargeResourceFields(int vpdFileFd, uint16_t resPos, uint16_t re g_autofree uint8_t *buf = g_malloc0(PCI_VPD_MAX_FIELD_SIZE + 1); uint16_t fieldDataLen = 0, bytesToRead = 0; uint16_t fieldPos = resPos; - bool hasChecksum = false; - bool hasRW = false; - bool endReached = false; /* Note the equal sign - fields may have a zero length in which case they will * just occupy 3 header bytes. In the in case of the RW field this may mean that @@ -507,7 +504,9 @@ virPCIVPDParseVPDLargeResourceFields(int vpdFileFd, uint16_t resPos, uint16_t re case VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_RDWR: /* Skip the read-write space since it is used for indication only. */ - hasRW = true; + /* The lack of RW is allowed on purpose in the read-write section since some vendors + * violate the PCI/PCIe specs and do not include it, however, this does not prevent parsing + * of valid data. */ goto done; case VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_RESVD: @@ -531,6 +530,7 @@ virPCIVPDParseVPDLargeResourceFields(int vpdFileFd, uint16_t resPos, uint16_t re if (!res->rw) res->rw = virPCIVPDResourceRWNew(); } + /* The field format, keyword and value are determined. Attempt to update the resource. */ virPCIVPDResourceUpdateKeyword(res, readOnly, fieldKeyword, fieldValue); } @@ -538,27 +538,21 @@ virPCIVPDParseVPDLargeResourceFields(int vpdFileFd, uint16_t resPos, uint16_t re done: /* May have exited the loop prematurely in case RV or RW were encountered and * they were not the last fields in the section. */ - endReached = (fieldPos >= resPos + resDataLen); - if (readOnly && !(hasChecksum && endReached)) { - VIR_DEBUG("VPD-R does not contain the mandatory RV field as the last field"); + if ((fieldPos < resPos + resDataLen)) { + /* unparsed data still present */ + VIR_DEBUG("PCI VPD data parsing failed"); + return false; + } + + if (readOnly && !hasChecksum) { + VIR_DEBUG("VPD-R does not contain the mandatory checksum"); return false; - } else if (!readOnly && !endReached) { - /* The lack of RW is allowed on purpose in the read-write section since some vendors - * violate the PCI/PCIe specs and do not include it, however, this does not prevent parsing - * of valid data. If the RW is present, however, we make sure it is the last field in - * the read-write section. */ - if (hasRW) { - VIR_DEBUG("VPD-W section parsing ended prematurely (RW is not the last field)."); - return false; - } else { - VIR_DEBUG("VPD-W section parsing ended prematurely."); - return false; - } } return true; } + /** * virPCIVPDParseVPDLargeResourceString: * @vpdFileFd: A file descriptor associated with a file containing PCI device VPD. -- 2.43.0