sound tests: Add SNDSTIOC_ADD_USER_DEVS test

[christos]

Summary

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

[dev_submerge.ch]

Can we somehow read these values back out, for checking them? Or where does this ioctl have any effect?

[christos]

In D46228#1054364, @dev_submerge.ch wrote:

Can we somehow read these values back out, for checking them? Or where does this ioctl have any effect?

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.

[dev_submerge.ch]

In D46228#1054412, @christos wrote:

In D46228#1054364, @dev_submerge.ch wrote:

Can we somehow read these values back out, for checking them? Or where does this ioctl have any effect?

So the device will get destroyed once we call close(fd).

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.

[christos]

Read back values. Introduce read_all() function.

[dev_submerge.ch]

I think we still don't test whether the userland device was in fact added to the list - am I missing something?

[christos]

In D46228#1054608, @dev_submerge.ch wrote:

I think we still don't test whether the userland device was in fact added to the list - am I missing something?

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.

[christos]

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

[christos]

Minor fix.

[dev_submerge.ch]

tests/sys/sound/sndstat.c

316

Could we go with ATF_CHECK() here and in the following data comparison cases?

[christos]

tests/sys/sound/sndstat.c

316

Is there a particular reason we'd prefer ATF_CHECK()?

[dev_submerge.ch]

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.

[christos]

tests/sys/sound/sndstat.c

316

Right, that makes sense. Will fix this.

[christos]

Use ATF_CHECK() in nvlist field checking.