Start adding basic tests for cam(3)

[ngie]

Summary

Start adding basic tests for cam(3)

This change contains several negative and positive tests for:

• cam_open_device
• cam_close_device
• cam_getccb
• cam_freeccb

This also contains a test for the failure case noted in bug 217649,
i.e., O_RDWR must be specified because pass(4) requires it.

This test unfortunately cannot assume that cam-capable devices
are present, so the user must explicitly provide a device via
test_suites.FreeBSD.cam_test_device. In the future, a test
kernel module might be shipped which will allow the tests to do
away with the requirement and instead rely on that kernel
module.

MFC after: 1 week
Sponsored by: Dell EMC Isilon

Test Plan

$ grep cam_test_device /usr/local/etc/kyua/kyua.conf
test_suites.FreeBSD.cam_test_device = '/dev/da0'
$ make -s clean; make -s; sudo make -s install; sudo make check
libcam_test:cam_close_device_negative_test_NULL  ->  passed  [0.003s]
libcam_test:cam_freeccb_negative_test_NULL  ->  passed  [0.003s]
libcam_test:cam_getccb_positive_test  ->  passed  [0.003s]
libcam_test:cam_open_device_negative_test_O_RDONLY  ->  passed  [0.004s]
libcam_test:cam_open_device_negative_test_nonexistent  ->  passed  [0.003s]
libcam_test:cam_open_device_negative_test_unprivileged  ->  passed  [0.004s]
libcam_test:cam_open_device_positive_test  ->  passed  [0.004s]

Results file id is usr_tests_lib_libcam.20170309-080952-993779
Results saved to /root/.kyua/store/results.usr_tests_lib_libcam.20170309-080952-993779.db

7/7 passed (0 failed)
$

[ken]

It is good to start adding tests for libcam. These look like a good start.

Note that cam_freeccb(3) has never been explicitly documented to work properly when passed a NULL pointer. It happens to work because it just calls free(3), which works that way. It is there primarily to provide symmetry with the cam_getccb(3) function, which actually does something on top of a malloc(3), and as a placeholder in case we want to add more functionality there later. (Cleaning up other allocated resources, for instance.)

So, I said all that to say that you may want to add a mention in the man page that cam_freeccb(3) allows NULL arguments if you're going to add a test verifying that behavior.

As for having the user provide a CAM device, you can do that completely in software with CTL. You could have the test create a CTL ramdisk or two (see ctladm(8)), run the tests, and tear them down. The challenge with that is just making sure you don't interfere with other targets the user has on his system. I guess that comes down to a question of what system state do we expect when the tests start running.

[asomers]

lib/libcam/Makefile

46 ↗

(On Diff #26091)

By convention, ".include <src.opts.mk>" goes near the top of the Makefile. Is there any reason why it must be lower here?

lib/libcam/tests/Makefile

7 ↗

(On Diff #26091)

6 is the default for WARNS. You can eliminate this line.

lib/libcam/tests/libcam_test.c

43 ↗

(On Diff #26091)

Can't you perform this check by putting a atf_tc_set_md_var(tc, "require.config", "cam_test_device") in the test head? Or by putting TEST_METADATA.$t+= required_config="cam_test_device" in the Makefile, for each test program?

81 ↗

(On Diff #26091)

How do you know that the device in question is a pass(4) device and not a da(4) device or something?

138 ↗

(On Diff #26091)

These could be ATF_CHECK_* instead of ATF_REQUIRE_*

[ngie]

lib/libcam/Makefile

46 ↗

(On Diff #26091)

Including it below LIBDIR so it doesn't automatically override LIBDIR to /usr/lib; we ran into a handful of silly mistakes like this at Isilon... I generally put it lower down to avoid said mistakes..

lib/libcam/tests/Makefile

7 ↗

(On Diff #26091)

Not true unfortunately, because ../Makefile.inc doesn't exist (that's how WARNS?= 6 gets set for lib/libcam/...). Example (on another directory I'm working on for lib/libkvm):

$ make -VWARNS

$ pwd
/usr/src/lib/libkvm/tests
$

I haven't pushed for WARNS?= 6 across the board yet for tests -- and I'm not sure I want to because it seems counterintuitive to the rest of our make logic/behavior, but I might push for a different hook to ensure that tests/... pulls in ../../Makefile.inc.

lib/libcam/tests/libcam_test.c

43 ↗

(On Diff #26091)

IIRC, no. Unfortunately kyua chokes on the "unknown variable" the last I tried. I'll retry when I get back from my mini staycation though (Saturday night).

138 ↗

(On Diff #26091)

Good point.

[ngie]

In D9928#205341, @ken wrote:

It is good to start adding tests for libcam. These look like a good start.

Note that cam_freeccb(3) has never been explicitly documented to work properly when passed a NULL pointer. It happens to work because it just calls free(3), which works that way. It is there primarily to provide symmetry with the cam_getccb(3) function, which actually does something on top of a malloc(3), and as a placeholder in case we want to add more functionality there later. (Cleaning up other allocated resources, for instance.)

So, I said all that to say that you may want to add a mention in the man page that cam_freeccb(3) allows NULL arguments if you're going to add a test verifying that behavior.

Will do!

As for having the user provide a CAM device, you can do that completely in software with CTL. You could have the test create a CTL ramdisk or two (see ctladm(8)), run the tests, and tear them down. The challenge with that is just making sure you don't interfere with other targets the user has on his system. I guess that comes down to a question of what system state do we expect when the tests start running.

Isilon used to have some working shell tests that used ctl(4), but unfortunately it broke with our OS upgrade from 10.x to 11.x.

ctl(4) has been overloaded to include iscsi(4), so unfortunately it's a non-starter for some (like Isilon). I haven't decoupled the iscsi parts from ctl(4) yet, but will likely do that, create a review, and commit it, if successful.

[asomers]

lib/libcam/tests/Makefile

7 ↗

(On Diff #26091)

Ahh, that's how it's done! I could never figure it out before. In that case, I have a few Makefiles to update.

lib/libcam/tests/libcam_test.c

43 ↗

(On Diff #26091)

Hm, kyuafile(5) suggests that it should be recognized, but the name is actually required_configs with an "s" on the end.

[ngie]

lib/libcam/tests/libcam_test.c

43 ↗

(On Diff #26091)

Delightfully inconsistent. Thanks for the heads up.

[ngie]

lib/libcam/tests/libcam_test.c

81 ↗

(On Diff #26091)

cam_open_device(3) doesn't make this distinction, at least per man 3 cam_open_device. This whole using a user-specified device is not 100% reliable. Adding in appropriate error checking in get_cam_test_device might be the best way to guard against this.

cam_open_device() takes as arguments a string describing the device it is
   to open, and flags suitable for passing to open(2).  The "path" passed in
   may actually be most any type of string that contains a device name and
   unit number to be opened.  The string will be parsed by cam_get_device()
   into a device name and unit number.  Once the device name and unit number
   are determined, a lookup is performed to determine the passthrough device
   that corresponds to the given device.

[ngie]

Use require.config file in testcase headers to enforce cam_test_device needing to be defined for some testcases

[ngie]

Use ATF_CHECK* for the first set of checks and ATF_REQUIRE* for the cam_has_error check

This allows the tester to develop a full picture of how/why things have failed, instead
of fixing one scenario, rerunning the test, and discovering that the other item needs
work.

The goal for the separate invariants is to make it clear which invariant has failed in
the test without introducing too much complexity in the test code.

[ngie]

Ping: @asomers: could you please provide feedback on the changes I've made?

[asomers]

lib/libcam/tests/libcam_test.c

81 ↗

(On Diff #26091)

So you're saying that it doesn't matter what device node the user specifies, as long as it's a CAM device? In that case, you should probably update descr to omit specific mentions of pass(4).

[ngie]

lib/libcam/tests/libcam_test.c

81 ↗

(On Diff #26091)

Ok, now I see what you were asking about. The issue was that issuing the ioctl with a readonly file descriptor would fail with EPERM as noted in bug 217649.

A scenario that I need to doublecheck is whether or not this test fails if pass support isn't built into the kernel/as a kld and loaded.

[ngie]

Disambiguate description for :cam_open_device_negative_test_O_RDONLY