Page MenuHomeFreeBSD

lib/libkvm: add kvm_read test
ClosedPublic

Authored by alfredo on Feb 21 2020, 4:38 PM.

Details

Summary

This adds a sanity check for the value received from kvm_read. Test will fail if value is different from the expected one.

This derived from work in D23776

Test Plan

run TestSuite on powerpc64 and amd64

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Can you also test this on amd64?

ngie requested changes to this revision.EditedFeb 24 2020, 4:49 PM

One thing that isn't clear is why this sanity check is needed. Is this being specifically for platform integration issue with PowerPC64, or is there another better reason for why this needs to be done given the API? If for the latter reason and the item is well-documented, please add it. If not, this doesn't seem to serve much value and might be better implemented as a platform-specific regression test.

Please test this on amd64 as well as a sanity check, second.

lib/libkvm/tests/kvm_geterr_test.c
30 ↗(On Diff #68640)

This doesn't follow style(9).

  1. sys/types.h isn't needed if sys/param.h is present.
  2. sys/sysctl.h should be below sys/param.h // sys/types.h .
This revision now requires changes to proceed.Feb 24 2020, 4:49 PM
In D23783#523422, @ngie wrote:

One thing that isn't clear is why this sanity check is needed. Is this being specifically for platform integration issue with PowerPC64, or is there another better reason for why this needs to be done given the API? If for the latter reason and the item is well-documented, please add it. If not, this doesn't seem to serve much value and might be better implemented as a platform-specific regression test.

Please test this on amd64 as well as a sanity check, second.

I sent an update to my previous comment that I wanted to note via email as well for clarity. I included it above.

jhb added inline comments.
lib/libkvm/tests/kvm_geterr_test.c
131 ↗(On Diff #68640)

I think verifying that kvm_Read() is returning the correct value is probably a decent test, and it should work on all platforms (mp_maxcpus is MI after all).

I think sticking it in this test is probably a bit dubious though. I know it would require some code duplication, but I think this should be a standalone test of kvm_read rather than part of this test for kvm_geterr.

lib/libkvm/tests/kvm_geterr_test.c
131 ↗(On Diff #68640)

@jhb: agreed.

In D23783#523422, @ngie wrote:

One thing that isn't clear is why this sanity check is needed. Is this being specifically for platform integration issue with PowerPC64, or is there another better reason for why this needs to be done given the API? If for the latter reason and the item is well-documented, please add it. If not, this doesn't seem to serve much value and might be better implemented as a platform-specific regression test.

Please test this on amd64 as well as a sanity check, second.

I added the test because while creating D23776 I initially prevented panic but while trying to understand the data returned I figured out the initial solution was completelly wrong. So I think a sanity check of the value would help us to quickly detect regression on both kvm_read and /dev/kmem read consequently. As suggested, I will rework this as a new test case.

It looks like the variable is not platform specific. I will double check the results on amd64.

lib/libkvm/tests/kvm_geterr_test.c
131 ↗(On Diff #68640)

Sure, I'm going to create a new test for this check.

alfredo marked an inline comment as done.
alfredo retitled this revision from lib/libkvm: add sanity check in kvm_geterr_positive_test_no_error test to lib/libkvm: add kvm_read test.
alfredo edited the summary of this revision. (Show Details)
alfredo edited the test plan for this revision. (Show Details)

Hi @ngie are you ok with the new "kvm_read_test" test?
I tested it on powerpc64 and amd64

lib/libkvm/tests/kvm_read_test.c
48 ↗(On Diff #68902)

This test doesn't need kvm_open2(). You can just use kvm_open() instead as that works fine for native. kvm_open2() is only needed to support cross-debugging of vmcores.

89 ↗(On Diff #68902)

This seems odd to test, and it requires you to abuse _kvm_err(). I think it would be better to just not bother testing this. Just as errno is only valid after an error, kvm_geterr() is only valid to call if you have an error. I would argue that calling it after success in this case is undefined behavior, so not something worth testing.

use kvm_open instead of kvm_open2

Remove call to internal API _kvm_err after reviewer's comment

@ngie, @jhb do you think it's ok to merge now? Thanks!

lib/libkvm/tests/kvm_read_test.c
89 ↗(On Diff #68902)

Sure, this was based on another test, but I agree removing it.

This revision was not accepted when it landed; it landed in state Needs Review.Jul 8 2020, 6:59 PM
Closed by commit rS363020: test: add libkvm read test (authored by alfredo). · Explain Why
This revision was automatically updated to reflect the committed changes.