lib/libkvm: Start adding basic tests for kvm(3)

[ngie]

Summary

lib/libkvm: Start adding basic tests for kvm(3)

• kvm_close: add a testcase to verify support for errno=EINVAL/-1 (see D10065) when kd == NULL is provided to the libcall.
• kvm_geterr:
  • add a negative testcase for kd == NULL returning "" (see D10022).
  • add two positive testcases:
    • test the error case using kvm_write on a O_RDONLY descriptor.
    • test the "no error" case using kvm_read(3) and kvm_nlist(3) as helper routines and by injecting a bogus error message via _kvm_err (an internal API) _kvm_err was used as there isn't a formalized way to clear the error output, and because kvm_nlist always returns ENOENT with the NULL terminator today.
• kvm_open, kvm_open2: add some basic negative tests for kvm_open(3) and kvm_open2(3). Testing positive cases with a specific corefile/execfile/resolver requires more work and would require user intervention today in order to reliably test this out.

MFC after: 1 month
Sponsored by: Dell EMC Isilon

[ngie]

Add more kvm_geterr(3) tests -- some positive ones this time!

[ngie]

Does anyone on the CC list have time to review these tests?

lib/libkvm/tests/kvm_open2_test.c

81 ↗

(On Diff #26307)

Noting errbuf here doesn't make sense.

[markj]

lib/libkvm/tests/kvm_open2_test.c

52 ↗

(On Diff #26307)

Shouldn't these tests clear errbuf? It looks like they're relying on zero initialization.

95 ↗

(On Diff #26307)

Would /dev/zero make more sense?

98 ↗

(On Diff #26307)

Why not check the return value of kvm_close()?

lib/libkvm/tests/kvm_open_test.c

93 ↗

(On Diff #26307)

Same here.

[ngie]

Address markj's comments:

• Add helper routines for clearing/testing errbuf
• Apply :kvm_open2_negative_test_invalid_execfile :
  • Use /dev/zero instead of /dev/null.
  • Check return code for kvm_close(3)

Also, in :kvm_open2_negative_test_invalid_corefile, remove errbuf
printing on unexpected success.

[ngie]

Add kvm_test_common.h missed in previous diff

[ngie]

Update per updates in D10022.

[ngie]

Ping?

[markj]

lib/libkvm/tests/kvm_close_test.c

26 ↗

(On Diff #26436)

.c files should use FBSDID.

lib/libkvm/tests/kvm_geterr_test.c

119 ↗

(On Diff #26436)

Looks like a debug print.

lib/libkvm/tests/kvm_open2_test.c

103 ↗

(On Diff #26436)

Hm, so opening /dev/zero doesn't result in an error? I don't really understand the purpose of this test - it just verifies that opening a non-ELF file together with /dev/mem doesn't result in an error. But in practice libkvm consumers will specify ELF files here, and if you specify a core file as well, kvm_open2() will fail.

[ngie]

lib/libkvm/tests/kvm_open2_test.c

103 ↗

(On Diff #26436)

Yeah, you're right... this is opposite of what this should really be. An invalid execfile should result in a failure because it doesn't have symbols.

[ngie]

lib/libkvm/tests/kvm_open2_test.c

103 ↗

(On Diff #26436)

Ok, I see why it was not tripping the error for me based on code inspection. The code assumes that because you feed in a core file of NULL (which then becomes /dev/mem), you are going to be running with a live system, as opposed to a dead system. Because of that reasoning, it returns kd early in the block starting at line 156.

Basically, I have to feed in a bogus real file, or ideally a valid file, for corefile.

[ngie]

Update per feedback from markj

The key difference with this change is the invalid execfile testcases now use
/bin/sh for the corefile (reason being that I can't unfortunately specify
a valid corefile for testing, yet... this is pending future commits after I
add positive cases for kvm_open*(3) using corefile/execfile)

Reason for this change is that _kvm_open (the underlying call for kvm_open(3))
bypasses execfile evaluation if corefile == NULL (it isn't explicitly stated in
the manpage -- this certainly deserves clarification).

[ngie]

Ugh... arc update from lib/libkvm, not lib/libkvm/tests...

[ngie]

Don't forget mtree changes included in previous reviews (sorry for the
spam!!)

[markj]

Seems ok, modulo the comments.

lib/libkvm/tests/kvm_close_test.c

26 ↗

(On Diff #26436)

Extra newline before __FBSDID.

lib/libkvm/tests/kvm_geterr_test.c

65 ↗

(On Diff #26704)

This description doesn't match what the test is doing.

113 ↗

(On Diff #26704)

Why not require kvm_nlist(...) > 0?

[ngie]

lib/libkvm/tests/kvm_close_test.c

26 ↗

(On Diff #26436)

Fixed all of the __FBSDID usage.

lib/libkvm/tests/kvm_geterr_test.c

65 ↗

(On Diff #26704)

I forgot to update this after changing libkvm to use "". Fixed the description.

113 ↗

(On Diff #26704)

For pedantic correctness on the negative case.

RETURN VALUES
     The kvm_nlist() and kvm_nlist2() functions return the number of invalid
     entries found.  If the kernel symbol table was unreadable, -1 is
     returned.

The code/manpage suggests that the number of entries should be returned, but doesn't seem to be the case empirically:

kvm_geterr_test:kvm_geterr_positive_test_no_error  ->  failed: /usr/src/lib/libkvm/tests/kvm_geterr_test.c:114: kvm_nlist failed (0): kvm_nlist: No such file or directory  [0.003s]

I'll dig into this further.