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@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 :|