External/libvirt
1
0
Fork 0
mirror of https://gitlab.com/libvirt/libvirt.git synced 2024-12-22 05:35:25 +00:00
Code Issues Packages Projects Releases Wiki Activity

util: virpcivpd: Remove return value from virPCIVPDResourceUpdateKeyword

Browse Source 
The function always succeeded and after the removal of programing error
checks doesn't even have a 'return false' case. Additionally one of the
tests in 'virpcivpdtest' tested that this function never failed on wrong
data. Embrace this logic and remove the return value and adjust logging
to VIR_DEBUG level to avoid spamming logs.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
This commit is contained in:
Peter Krempa 2024-01-24 17:15:10 +01:00
parent dd36db2607
commit 9aa303a948
3 changed files with 34 additions and 43 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

31
src/util/virpcivpd.c
View File

@ -307,54 +307,48 @@ virPCIVPDResourceCustomUpsertValue(GPtrArray *arr, char index, const char *const
* used in XML elements. For vendor-specific and system-specific keywords only V%s and Y%s * used in XML elements. For vendor-specific and system-specific keywords only V%s and Y%s
* (except "YA" which is an asset tag) formatted values are accepted. * (except "YA" which is an asset tag) formatted values are accepted.
* *
* Returns: true if a keyword has been updated successfully, false otherwise. * Unknown or malformed values are ignored.
*/ */
bool void
virPCIVPDResourceUpdateKeyword(virPCIVPDResource *res, const bool readOnly, virPCIVPDResourceUpdateKeyword(virPCIVPDResource *res,
const char *const keyword, const char *const value) const bool readOnly,
const char *const keyword,
const char *const value)
{ {
if (readOnly) { if (readOnly) {
if (STREQ("EC", keyword) || STREQ("change_level", keyword)) { if (STREQ("EC", keyword) || STREQ("change_level", keyword)) {
g_free(res->ro->change_level); g_free(res->ro->change_level);
res->ro->change_level = g_strdup(value); res->ro->change_level = g_strdup(value);
return true;
} else if (STREQ("MN", keyword) || STREQ("manufacture_id", keyword)) { } else if (STREQ("MN", keyword) || STREQ("manufacture_id", keyword)) {
g_free(res->ro->manufacture_id); g_free(res->ro->manufacture_id);
res->ro->manufacture_id = g_strdup(value); res->ro->manufacture_id = g_strdup(value);
return true;
} else if (STREQ("PN", keyword) || STREQ("part_number", keyword)) { } else if (STREQ("PN", keyword) || STREQ("part_number", keyword)) {
g_free(res->ro->part_number); g_free(res->ro->part_number);
res->ro->part_number = g_strdup(value); res->ro->part_number = g_strdup(value);
return true;
} else if (STREQ("SN", keyword) || STREQ("serial_number", keyword)) { } else if (STREQ("SN", keyword) || STREQ("serial_number", keyword)) {
g_free(res->ro->serial_number); g_free(res->ro->serial_number);
res->ro->serial_number = g_strdup(value); res->ro->serial_number = g_strdup(value);
return true;
} else if (virPCIVPDResourceIsVendorKeyword(keyword)) { } else if (virPCIVPDResourceIsVendorKeyword(keyword)) {
virPCIVPDResourceCustomUpsertValue(res->ro->vendor_specific, keyword[1], value); virPCIVPDResourceCustomUpsertValue(res->ro->vendor_specific, keyword[1], value);
return true;
} else if (STREQ("FG", keyword) || STREQ("LC", keyword) || STREQ("PG", keyword)) { } else if (STREQ("FG", keyword) || STREQ("LC", keyword) || STREQ("PG", keyword)) {
/* Legacy PICMIG keywords are skipped on purpose. */ /* Legacy PICMIG keywords are skipped on purpose. */
return true;
} else if (STREQ("CP", keyword)) { } else if (STREQ("CP", keyword)) {
/* The CP keyword is currently not supported and is skipped. */ /* The CP keyword is currently not supported and is skipped. */
return true; } else {
VIR_DEBUG("unhandled PCI VPD r/o keyword '%s'(val='%s')", keyword, value);
} }
} else { } else {
if (STREQ("YA", keyword) || STREQ("asset_tag", keyword)) { if (STREQ("YA", keyword) || STREQ("asset_tag", keyword)) {
g_free(res->rw->asset_tag); g_free(res->rw->asset_tag);
res->rw->asset_tag = g_strdup(value); res->rw->asset_tag = g_strdup(value);
return true;
} else if (virPCIVPDResourceIsVendorKeyword(keyword)) { } else if (virPCIVPDResourceIsVendorKeyword(keyword)) {
virPCIVPDResourceCustomUpsertValue(res->rw->vendor_specific, keyword[1], value); virPCIVPDResourceCustomUpsertValue(res->rw->vendor_specific, keyword[1], value);
return true;
} else if (virPCIVPDResourceIsSystemKeyword(keyword)) { } else if (virPCIVPDResourceIsSystemKeyword(keyword)) {
virPCIVPDResourceCustomUpsertValue(res->rw->system_specific, keyword[1], value); virPCIVPDResourceCustomUpsertValue(res->rw->system_specific, keyword[1], value);
return true; } else {
VIR_DEBUG("unhandled PCI VPD r/w keyword '%s'(val='%s')", keyword, value);
} }
} }
VIR_WARN("Tried to update an unsupported keyword %s: skipping.", keyword);
return true;
} }
#ifdef __linux__ #ifdef __linux__
@ -527,10 +521,7 @@ virPCIVPDParseVPDLargeResourceFields(int vpdFileFd, uint16_t resPos, uint16_t re
res->rw = virPCIVPDResourceRWNew(); res->rw = virPCIVPDResourceRWNew();
} }
/* The field format, keyword and value are determined. Attempt to update the resource. */ /* The field format, keyword and value are determined. Attempt to update the resource. */
if (!virPCIVPDResourceUpdateKeyword(res, readOnly, fieldKeyword, fieldValue)) { virPCIVPDResourceUpdateKeyword(res, readOnly, fieldKeyword, fieldValue);
VIR_INFO("Could not update the VPD resource keyword: %s", fieldKeyword);
return false;
}
} }
/* May have exited the loop prematurely in case RV or RW were encountered and /* May have exited the loop prematurely in case RV or RW were encountered and

8
src/util/virpcivpd.h
View File

@ -67,9 +67,11 @@ void virPCIVPDResourceRWFree(virPCIVPDResourceRW *rw);
G_DEFINE_AUTOPTR_CLEANUP_FUNC(virPCIVPDResourceRW, virPCIVPDResourceRWFree); G_DEFINE_AUTOPTR_CLEANUP_FUNC(virPCIVPDResourceRW, virPCIVPDResourceRWFree);
bool void
virPCIVPDResourceUpdateKeyword(virPCIVPDResource *res, const bool readOnly, virPCIVPDResourceUpdateKeyword(virPCIVPDResource *res,
const char *const keyword, const char *const value); const bool readOnly,
const char *const keyword,
const char *const value);
void virPCIVPDResourceCustomFree(virPCIVPDResourceCustom *custom); void virPCIVPDResourceCustomFree(virPCIVPDResourceCustom *custom);

32
tests/virpcivpdtest.c
View File

@ -84,17 +84,15 @@ testPCIVPDResourceBasic(const void *data G_GNUC_UNUSED)
/* Update keywords one by one and compare actual values with the expected ones. */ /* Update keywords one by one and compare actual values with the expected ones. */
for (i = 0; i < numROCases; ++i) { for (i = 0; i < numROCases; ++i) {
if (!virPCIVPDResourceUpdateKeyword(res, true, virPCIVPDResourceUpdateKeyword(res, true,
readOnlyCases[i].keyword, readOnlyCases[i].keyword,
readOnlyCases[i].value)) readOnlyCases[i].value);
return -1;
if (STRNEQ(readOnlyCases[i].value, *readOnlyCases[i].actual)) if (STRNEQ(readOnlyCases[i].value, *readOnlyCases[i].actual))
return -1; return -1;
} }
/* Do a basic vendor field check. */ /* Do a basic vendor field check. */
if (!virPCIVPDResourceUpdateKeyword(res, true, "V0", "vendor0")) virPCIVPDResourceUpdateKeyword(res, true, "V0", "vendor0");
return -1;
if (res->ro->vendor_specific->len != 1) if (res->ro->vendor_specific->len != 1)
return -1; return -1;
@ -105,25 +103,23 @@ testPCIVPDResourceBasic(const void *data G_GNUC_UNUSED)
/* Make sure unsupported RO keyword updates are not fatal. */ /* Make sure unsupported RO keyword updates are not fatal. */
for (i = 0; i < numUnsupportedCases; ++i) { for (i = 0; i < numUnsupportedCases; ++i) {
if (!virPCIVPDResourceUpdateKeyword(res, true, virPCIVPDResourceUpdateKeyword(res, true,
unsupportedFieldCases[i].keyword, unsupportedFieldCases[i].keyword,
unsupportedFieldCases[i].value)) unsupportedFieldCases[i].value);
return -1;
} }
/* Initialize RW */ /* Initialize RW */
res->rw = g_steal_pointer(&rw); res->rw = g_steal_pointer(&rw);
if (!virPCIVPDResourceUpdateKeyword(res, false, "YA", "tag1") virPCIVPDResourceUpdateKeyword(res, false, "YA", "tag1");
|| STRNEQ(res->rw->asset_tag, "tag1")) if (STRNEQ(res->rw->asset_tag, "tag1"))
return -1; return -1;
if (!virPCIVPDResourceUpdateKeyword(res, false, "asset_tag", "tag2") virPCIVPDResourceUpdateKeyword(res, false, "asset_tag", "tag2");
|| STRNEQ(res->rw->asset_tag, "tag2")) if (STRNEQ(res->rw->asset_tag, "tag2"))
return -1; return -1;
/* Do a basic system field check. */ /* Do a basic system field check. */
if (!virPCIVPDResourceUpdateKeyword(res, false, "Y0", "system0")) virPCIVPDResourceUpdateKeyword(res, false, "Y0", "system0");
return -1;
if (res->rw->system_specific->len != 1) if (res->rw->system_specific->len != 1)
return -1; return -1;
@ -134,10 +130,12 @@ testPCIVPDResourceBasic(const void *data G_GNUC_UNUSED)
/* Make sure unsupported RW keyword updates are not fatal. */ /* Make sure unsupported RW keyword updates are not fatal. */
for (i = 0; i < numUnsupportedCases; ++i) { for (i = 0; i < numUnsupportedCases; ++i) {
if (!virPCIVPDResourceUpdateKeyword(res, false, /* This test is deliberately left in despite
* virPCIVPDResourceUpdateKeyword always succeeding to prevent
* possible regressions if the function is ever rewritten */
virPCIVPDResourceUpdateKeyword(res, false,
unsupportedFieldCases[i].keyword, unsupportedFieldCases[i].keyword,
unsupportedFieldCases[i].value)) unsupportedFieldCases[i].value);
return -1;
} }