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