Test whether the SNDSTIOC_ADD_USER_DEVS IOCTL (registers a userland
device to /dev/sndstat) works properly.
Sponsored by: The FreeBSD Foundation
MFC after: 2 days
Details
Details
Diff Detail
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Skipped - Unit
Tests Skipped - Build Status
Buildable 58970 Build 55857: arc lint + arc unit
Event Timeline
Comment Actions
Can we somehow read these values back out, for checking them? Or where does this ioctl have any effect?
Comment Actions
So the device will get destroyed once we call close(fd). Even though the sndstat_nv test case does the reading checks already, we could try and read the values back after we issue SNDSTIOC_ADD_USER_DEVS, I just didn't do this because I found it redundant to do what the other test case does already. The IOCTL won't succeed if the device is not registered properly.
Comment Actions
For my understanding of how this ioctl works: This means that virtual_oss has to keep /dev/sndstat open as long as its userland pcm device is present, right?
Even though the sndstat_nv test case does the reading checks already, we could try and read the values back after we issue SNDSTIOC_ADD_USER_DEVS, I just didn't do this because I found it redundant to do what the other test case does already. The IOCTL won't succeed if the device is not registered properly.
Sure, we can just test that the ioctl doesn't return an error for given data. But a test of this kind can go trivially wrong if the ioctl returns a false positive, for whatever reason. It may give a false level of confidence to someone not reading the test code.
I'd suggest to at least check whether the device was added to the list.
Comment Actions
I think we still don't test whether the userland device was in fact added to the list - am I missing something?
Comment Actions
Indeed. I modified the diff a few hours ago to search for the userland device and read back all the values we registered, and compare them to what we passed to make sure all values match, but haven't had time to clean it up and submit it. I will submit the diff once I am on my computer soon.
Comment Actions
(Was AFK and couldn't submit earlier). Read back registered values and make
sure they match the ones we passed to SNDSTIOC_ADD_USER_DEVS.
| tests/sys/sound/sndstat.c | ||
|---|---|---|
| 316 | Could we go with ATF_CHECK() here and in the following data comparison cases? | |
| tests/sys/sound/sndstat.c | ||
|---|---|---|
| 316 | Is there a particular reason we'd prefer ATF_CHECK()? | |
| tests/sys/sound/sndstat.c | ||
|---|---|---|
| 316 | Good practice is to only use ATF_REQUIRE when the test cannot proceed in a meaningful way after the condition fails. Otherwise you should always prefer ATF_CHECK, to let the test advance and report as many errors as possible. This often helps when fixing multiple errors found by a test, or to understand the underlying problem. Therefore it's ok to use ATF_REQUIRE for the presence of the list or a specific field, since we cannot compare any values if they are absent. But if a value is not what the test expects, we can still go on and check the other values, using ATF_CHECK. | |
| tests/sys/sound/sndstat.c | ||
|---|---|---|
| 316 | Right, that makes sense. Will fix this. | |