libvirt/libvirt-virPCIVPDParseVPDLargeResourceFields-Refactor-return-logic.patch

From 72da25da974a1fa9d995b46afb76714d56f4cb9b Mon Sep 17 00:00:00 2001
Message-ID: <72da25da974a1fa9d995b46afb76714d56f4cb9b.1707394627.git.jdenemar@redhat.com>
From: Peter Krempa <pkrempa@redhat.com>
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 <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
(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