Details

Reviewers

jhibbits
ngie
jhb

Group Reviewers

tests

Commits

rS363990: MFC r363020: test: add libkvm read test
rS363020: test: add libkvm read test

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

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 29650
Build 27508: arc lint + arc unit

Event Timeline

alfredo created this revision.Feb 21 2020, 4:38 PM

Herald added subscribers: luporl, bdragon. · View Herald TranscriptFeb 21 2020, 4:38 PM

Harbormaster completed remote builds in B29521: Diff 68640.Feb 21 2020, 4:38 PM

alfredo mentioned this in D23776: [PowerPC] fix panic reading /dev/kmem on !DMAP machines.Feb 21 2020, 4:40 PM

Can you also test this on amd64?

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). sys/types.h isn't needed if sys/param.h is present. 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 a subscriber: jhb.Feb 24 2020, 8:05 PM

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`.

ngie added inline comments.Feb 26 2020, 4:08 AM

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 updated this revision to Diff 68902.Feb 27 2020, 7:15 PM

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)

Harbormaster completed remote builds in B29650: Diff 68902.Feb 27 2020, 7:15 PM

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

jhb added inline comments.Mar 26 2020, 5:34 PM

lib/libkvm/tests/kvm_read_test.c
48	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	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.