On Wed, Jan 31, 2024 at 09:06:06 +0100, Ján Tomko wrote:
On a Tuesday in 2024, Peter Krempa wrote:
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> --- src/util/virpcivpd.c | 31 +++++++++++-------------------- src/util/virpcivpd.h | 8 +++++--- tests/virpcivpdtest.c | 38 ++++++++++++++++++-------------------- 3 files changed, 34 insertions(+), 43 deletions(-)
diff --git a/tests/virpcivpdtest.c b/tests/virpcivpdtest.c index 20545759d5..a6311bfe76 100644 --- a/tests/virpcivpdtest.c +++ b/tests/virpcivpdtest.c @@ -134,10 +130,12 @@ testPCIVPDResourceBasic(const void *data G_GNUC_UNUSED)
/* Make sure unsupported RW keyword updates are not fatal. */ for (i = 0; i < numUnsupportedCases; ++i) { - if (!virPCIVPDResourceUpdateKeyword(res, false, - unsupportedFieldCases[i].keyword, - unsupportedFieldCases[i].value)) - return -1; + /* This test is deliberately left in despite + * virPCIVPDResourceUpdateKeyword always succeeding to prevent + * possible regressions if the function is ever rewritten */
How is this different from any of the pointless tests you removed earlier?
Good question. Originally this was the code that reminded me that I can't just add proper error reporting to that function which I did originally. I decided to keep this to preserve this fact in the code, because the comments/documentation for this particular code are/were in many cases misleading. I guess converting this expectation into a comment would be possible, which would allow us to remove the test. I can do this as a follow up if you are okay with this solution.