Re: [libvirt PATCH 3/4] PCI VPD: Skip fields with invalid values
On Fri, Oct 22, 2021 at 03:45:44PM +0300, Dmitrii Shcherbakov wrote:
While invalid values need to be ignored when presenting VPD data to
the
user, it would be good to attempt to parse a valid portion of the VPD
instead of marking it invalid as a whole.
The particular example encountered on real hardware was twofold:
* "N/A" strings present in read-only fields. This would not be a useful
valid value for a field, not to mention that if the serial number
field with the "N/A" value was parsed, it would confuse higher-level
software because this isn't a unique serial for a device;
The higher level software needs to acccept that the BIOS vendor
might have included garbage and have a plan to deal with this.
Libvirt should apply a policy in this regards as it cann result
in libvirt rejecting valid data.
For example by rejecting "/" as a character, we fail to report
valid data from mymachine.
If I allow "/" I get:
<capability type='vpd'>
<name>HP Ethernet 1Gb 2-port 361i Adapter</name>
<fields access='readonly'>
<change_level>N/A</change_level>
<part_number>N/A</part_number>
<serial_number>N/A</serial_number>
<vendor_field index='0'>4W/1W PCIeG2x4 2p 1GbE RJ45 Intel
i350</vendor_field>
</fields>
<fields access='readwrite'>
<asset_tag>N/A</asset_tag>
<vendor_field index='1'>5.7.06</vendor_field>
<vendor_field index='3'>2.8.20</vendor_field>
<vendor_field index='6'>1.5.35</vendor_field>
</fields>
</capability>
sure the "n/a" is unhelpful, but libvirt is right to
include it.
I think perhaps rather than allow-listing the characters, we need
to just accept any field value that contains printable characters.
eg instead of the strspn, I'd suggest we do as "isprint()" check
on each character
* 0xFF bytes present in VPD-W field values. Those bytes are not
valid
values and were probably used by the vendor as placeholders. Ignoring
the whole VPD because of that would be too strict.
Signed-off-by: Dmitrii Shcherbakov <dmitrii.shcherbakov(a)canonical.com>
---
src/util/virpcivpd.c | 9 ++--
tests/virpcivpdtest.c | 105 ++++++++++++++++++++++++++++++++++++++----
2 files changed, 102 insertions(+), 12 deletions(-)
diff --git a/src/util/virpcivpd.c b/src/util/virpcivpd.c
index cd49031fa4..8c2b17c3a6 100644
--- a/src/util/virpcivpd.c
+++ b/src/util/virpcivpd.c
@@ -544,9 +544,12 @@ virPCIVPDParseVPDLargeResourceFields(int vpdFileFd, uint16_t resPos,
uint16_t re
*/
fieldValue = g_strstrip(g_strndup((char *)buf, fieldDataLen));
if (!virPCIVPDResourceIsValidTextValue(fieldValue)) {
This method itself calls virReportError, so we still get stuff reported at
level ERROR in the logs. We need to remove the virReportError call in
this method.
- virReportError(VIR_ERR_INTERNAL_ERROR,
"%s",
- _("Field value contains invalid characters"));
- return false;
+ /* Skip fields with invalid values - this is safe assuming field length
is
+ * correctly specified. */
+ VIR_DEBUG("A value for field %s contains invalid characters",
fieldKeyword);
We should include fieldValue in the log, even if it might have non-printable
chars, as it'll be important debugging info.
+ g_free(g_steal_pointer(&fieldKeyword));
+ g_free(g_steal_pointer(&fieldValue));
+ continue;
}
} else if (fieldFormat == VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_RESVD) {
if (*csum) {
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|