On 12/8/21 11:39 AM, Mark Brown wrote:
On Wed, Dec 08, 2021 at 10:42:35AM -0700, Shuah Khan wrote:
On 12/6/21 9:03 AM, Mark Brown wrote:
+SOUND - ALSA SELFTESTS +M: Mark Brown broonie@kernel.org +L: alsa-devel@alsa-project.org (moderated for non-subscribers)
Please add linux-kselftest list as well here.
get_maintainers pulls it in from the wider entry (the mention of alsa-devel is reudnant too).
+int num_cards = 0; +int num_controls = 0; +struct card_data *card_list = NULL; +struct ctl_data *ctl_list = NULL;
No need to initailize the above globals.
They're not declared static so the initial value is undefined.
+void find_controls(void) +{
- char name[32];
Use SYSFS_PATH_MAX = 255 like other tools do?
This isn't a path, it's an ALSA limit for a name that is embedded in a struct (snd_ctl_card_info->name). There's no magic define for these lengths.
ctl_data->next = ctl_list;ctl_list = ctl_data;}- next_card:
No need to indent the label
No need but it looks wrong otherwise - it's certainly what I'd expect for normal kernel code.
- if (snd_ctl_elem_info_is_inactive(ctl->info)) {
ksft_print_msg("%s is inactive\n", ctl->name);ksft_test_result_skip("get_value.%d.%d\n",ctl->card->card, ctl->elem);The two messages could be combined?
The user facing control names almost always have spaces in them so while it's useful to have them for diagnostic purposes I don't think it's a good idea to put them in the KTAP test names, that's likely to confuse things trying to work with the KTAP output. The general pattern I'm following for these tests is to print one or more diagnostic lines explaining why a tests was skipped or failed with the human readable control name so people can hopefully figure out what's going on and have the KTAP facing name that tooling will deal with be a consistent test.card.control format for parsers and databases dealing with test results en masse.
Fine with me.
+bool test_ctl_write_valid_boolean(struct ctl_data *ctl) +{
- int err, i, j;
- bool fail = false;
- snd_ctl_elem_value_t *val;
Add blank line after declarations.
- snd_ctl_elem_value_alloca(&val);
This is idiomatic for alsa-lib code.
This is kernel code that is going into kernel sources. Why follow alsa-lib convention?
+int main(void) +{
- struct ctl_data *ctl;
- ksft_print_header();
Add a check for root and skil the test.
There is no need for this test to run as root in most configurations, it is common to provide direct access to the sound cards to some or all users - for example with desktop distros the entire userspace audio subsystem normally runs as the logged in user by default. alsa-lib's card enumeration should deal with any permissions problems accessing cards in the system cleanly. If the user running the test can't access any cards or the cards that can be accessed don't have any controls to test then we will find no controls during enumeration, report a plan to do zero tests and then exit cleanly which seems reasonable to me. If we do need to explicitly bomb out rather than report zero tests we should be doing it based on the number of controls we find rather than the user we're running as.
On my system, I don't see any output if run as root. Are there some tests that work as non-root?
thanks, -- Shuah