diff mbox series

[v1,1/2] kselftest: alsa: Factor out check that values meet constraints

Message ID 20211217130213.3893415-2-broonie@kernel.org (mailing list archive)
State New, archived
Headers show
Series kselftest: alsa: Small enhancements to mixer-test | expand

Commit Message

Mark Brown Dec. 17, 2021, 1:02 p.m. UTC 
To simplify the code a bit and allow future reuse factor the checks that
values we read are valid out of test_ctl_get_value() into a separate
function which can be reused later. As part of this extend the test to
check all the values for the control, not just the first one.

Signed-off-by: Mark Brown <broonie@kernel.org>
---
 tools/testing/selftests/alsa/mixer-test.c | 141 +++++++++++++---------
 1 file changed, 82 insertions(+), 59 deletions(-)

Comments

Cezary Rojewski Dec. 17, 2021, 1:32 p.m. UTC | #1 
On 2021-12-17 2:02 PM, Mark Brown wrote:
> To simplify the code a bit and allow future reuse factor the checks that
> values we read are valid out of test_ctl_get_value() into a separate
> function which can be reused later. As part of this extend the test to
> check all the values for the control, not just the first one.
> 
> Signed-off-by: Mark Brown <broonie@kernel.org>

...

> +/*
> + * Check that the provided value meets the constraints for the
> + * provided control.
> + */
> +bool ctl_value_valid(struct ctl_data *ctl, snd_ctl_elem_value_t *val)
> +{
> +	int i;
> +	bool valid = true;
> +
> +	for (i = 0; i < snd_ctl_elem_info_get_count(ctl->info); i++)
> +		if (!ctl_value_index_valid(ctl, val, i))
> +			valid = false;

Correct me I'm wrong, but it seems a 'return false' would suffice. Is 
the continuation of looping still needed once a single check found above 
evaluates to true?


Regards,
Czarek

> +
> +	return valid;
> +}
Mark Brown Dec. 17, 2021, 2:38 p.m. UTC | #2 
On Fri, Dec 17, 2021 at 02:32:24PM +0100, Cezary Rojewski wrote:
> On 2021-12-17 2:02 PM, Mark Brown wrote:

> > +	for (i = 0; i < snd_ctl_elem_info_get_count(ctl->info); i++)
> > +		if (!ctl_value_index_valid(ctl, val, i))
> > +			valid = false;

> Correct me I'm wrong, but it seems a 'return false' would suffice. Is the
> continuation of looping still needed once a single check found above
> evaluates to true?

It doesn't affect the result of the test but it will cause us to print a
diagnostic message for each invalid value rather than just the first one
we see (eg, if both channels in a stereo control have an invalid value)
which seems like it's more helpful to people working with the output
than just the first error.
Cezary Rojewski Dec. 17, 2021, 2:54 p.m. UTC | #3 
On 2021-12-17 3:38 PM, Mark Brown wrote:
> On Fri, Dec 17, 2021 at 02:32:24PM +0100, Cezary Rojewski wrote:
>> On 2021-12-17 2:02 PM, Mark Brown wrote:
> 
>>> +	for (i = 0; i < snd_ctl_elem_info_get_count(ctl->info); i++)
>>> +		if (!ctl_value_index_valid(ctl, val, i))
>>> +			valid = false;
> 
>> Correct me I'm wrong, but it seems a 'return false' would suffice. Is the
>> continuation of looping still needed once a single check found above
>> evaluates to true?

Just read my initial message and clearly an 'if' : (. Sorry for the 
confusion.

> It doesn't affect the result of the test but it will cause us to print a
> diagnostic message for each invalid value rather than just the first one
> we see (eg, if both channels in a stereo control have an invalid value)
> which seems like it's more helpful to people working with the output
> than just the first error.

So there is a good reason to do so. And I agree with such approach. 
Thanks for explaining, Mark!


Regards,
Czarek
diff mbox series

Patch

diff --git a/tools/testing/selftests/alsa/mixer-test.c b/tools/testing/selftests/alsa/mixer-test.c
index b798a76f6825..b009fc5df605 100644
--- a/tools/testing/selftests/alsa/mixer-test.c
+++ b/tools/testing/selftests/alsa/mixer-test.c
@@ -193,124 +193,147 @@  void find_controls(void)
 	snd_config_delete(config);
 }
 
-/*
- * Check that we can read the default value and it is valid. Write
- * tests use the read value to restore the default.
- */
-void test_ctl_get_value(struct ctl_data *ctl)
+bool ctl_value_index_valid(struct ctl_data *ctl, snd_ctl_elem_value_t *val,
+			   int index)
 {
-	int err;
 	long int_val;
 	long long int64_val;
 
-	/* If the control is turned off let's be polite */
-	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);
-		return;
-	}
-
-	/* Can't test reading on an unreadable control */
-	if (!snd_ctl_elem_info_is_readable(ctl->info)) {
-		ksft_print_msg("%s is not readable\n", ctl->name);
-		ksft_test_result_skip("get_value.%d.%d\n",
-				      ctl->card->card, ctl->elem);
-		return;
-	}
-
-	err = snd_ctl_elem_read(ctl->card->handle, ctl->def_val);
-	if (err < 0) {
-		ksft_print_msg("snd_ctl_elem_read() failed: %s\n",
-			       snd_strerror(err));
-		goto out;
-	}
-
 	switch (snd_ctl_elem_info_get_type(ctl->info)) {
 	case SND_CTL_ELEM_TYPE_NONE:
-		ksft_print_msg("%s Invalid control type NONE\n", ctl->name);
-		err = -1;
-		break;
+		ksft_print_msg("%s.%d Invalid control type NONE\n",
+			       ctl->name, index);
+		return false;
 
 	case SND_CTL_ELEM_TYPE_BOOLEAN:
-		int_val = snd_ctl_elem_value_get_boolean(ctl->def_val, 0);
+		int_val = snd_ctl_elem_value_get_boolean(val, index);
 		switch (int_val) {
 		case 0:
 		case 1:
 			break;
 		default:
-			ksft_print_msg("%s Invalid boolean value %ld\n",
-				       ctl->name, int_val);
-			err = -1;
-			break;
+			ksft_print_msg("%s.%d Invalid boolean value %ld\n",
+				       ctl->name, index, int_val);
+			return false;
 		}
 		break;
 
 	case SND_CTL_ELEM_TYPE_INTEGER:
-		int_val = snd_ctl_elem_value_get_integer(ctl->def_val, 0);
+		int_val = snd_ctl_elem_value_get_integer(val, index);
 
 		if (int_val < snd_ctl_elem_info_get_min(ctl->info)) {
-			ksft_print_msg("%s value %ld less than minimum %ld\n",
-				       ctl->name, int_val,
+			ksft_print_msg("%s.%d value %ld less than minimum %ld\n",
+				       ctl->name, index, int_val,
 				       snd_ctl_elem_info_get_min(ctl->info));
-			err = -1;
+			return false;
 		}
 
 		if (int_val > snd_ctl_elem_info_get_max(ctl->info)) {
-			ksft_print_msg("%s value %ld more than maximum %ld\n",
-				       ctl->name, int_val,
+			ksft_print_msg("%s.%d value %ld more than maximum %ld\n",
+				       ctl->name, index, int_val,
 				       snd_ctl_elem_info_get_max(ctl->info));
-			err = -1;
+			return false;
 		}
 
 		/* Only check step size if there is one and we're in bounds */
-		if (err >= 0 && snd_ctl_elem_info_get_step(ctl->info) &&
+		if (snd_ctl_elem_info_get_step(ctl->info) &&
 		    (int_val - snd_ctl_elem_info_get_min(ctl->info) %
 		     snd_ctl_elem_info_get_step(ctl->info))) {
-			ksft_print_msg("%s value %ld invalid for step %ld minimum %ld\n",
-				       ctl->name, int_val,
+			ksft_print_msg("%s.%d value %ld invalid for step %ld minimum %ld\n",
+				       ctl->name, index, int_val,
 				       snd_ctl_elem_info_get_step(ctl->info),
 				       snd_ctl_elem_info_get_min(ctl->info));
-			err = -1;
+			return false;
 		}
 		break;
 
 	case SND_CTL_ELEM_TYPE_INTEGER64:
-		int64_val = snd_ctl_elem_value_get_integer64(ctl->def_val, 0);
+		int64_val = snd_ctl_elem_value_get_integer64(val, index);
 
 		if (int64_val < snd_ctl_elem_info_get_min64(ctl->info)) {
-			ksft_print_msg("%s value %lld less than minimum %lld\n",
-				       ctl->name, int64_val,
+			ksft_print_msg("%s.%d value %lld less than minimum %lld\n",
+				       ctl->name, index, int64_val,
 				       snd_ctl_elem_info_get_min64(ctl->info));
-			err = -1;
+			return false;
 		}
 
 		if (int64_val > snd_ctl_elem_info_get_max64(ctl->info)) {
-			ksft_print_msg("%s value %lld more than maximum %lld\n",
-				       ctl->name, int64_val,
+			ksft_print_msg("%s.%d value %lld more than maximum %lld\n",
+				       ctl->name, index, int64_val,
 				       snd_ctl_elem_info_get_max(ctl->info));
-			err = -1;
+			return false;
 		}
 
 		/* Only check step size if there is one and we're in bounds */
-		if (err >= 0 && snd_ctl_elem_info_get_step64(ctl->info) &&
+		if (snd_ctl_elem_info_get_step64(ctl->info) &&
 		    (int64_val - snd_ctl_elem_info_get_min64(ctl->info)) %
 		    snd_ctl_elem_info_get_step64(ctl->info)) {
-			ksft_print_msg("%s value %lld invalid for step %lld minimum %lld\n",
-				       ctl->name, int64_val,
+			ksft_print_msg("%s.%d value %lld invalid for step %lld minimum %lld\n",
+				       ctl->name, index, int64_val,
 				       snd_ctl_elem_info_get_step64(ctl->info),
 				       snd_ctl_elem_info_get_min64(ctl->info));
-			err = -1;
+			return false;
 		}
 		break;
 
 	default:
 		/* No tests for other types */
+		break;
+	}
+
+	return true;
+}
+
+/*
+ * Check that the provided value meets the constraints for the
+ * provided control.
+ */
+bool ctl_value_valid(struct ctl_data *ctl, snd_ctl_elem_value_t *val)
+{
+	int i;
+	bool valid = true;
+
+	for (i = 0; i < snd_ctl_elem_info_get_count(ctl->info); i++)
+		if (!ctl_value_index_valid(ctl, val, i))
+			valid = false;
+
+	return valid;
+}
+
+/*
+ * Check that we can read the default value and it is valid. Write
+ * tests use the read value to restore the default.
+ */
+void test_ctl_get_value(struct ctl_data *ctl)
+{
+	int err;
+
+	/* If the control is turned off let's be polite */
+	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);
+		return;
+	}
+
+	/* Can't test reading on an unreadable control */
+	if (!snd_ctl_elem_info_is_readable(ctl->info)) {
+		ksft_print_msg("%s is not readable\n", ctl->name);
 		ksft_test_result_skip("get_value.%d.%d\n",
 				      ctl->card->card, ctl->elem);
 		return;
 	}
 
+	err = snd_ctl_elem_read(ctl->card->handle, ctl->def_val);
+	if (err < 0) {
+		ksft_print_msg("snd_ctl_elem_read() failed: %s\n",
+			       snd_strerror(err));
+		goto out;
+	}
+
+	if (!ctl_value_valid(ctl, ctl->def_val))
+		err = -EINVAL;
+
 out:
 	ksft_test_result(err >= 0, "get_value.%d.%d\n",
 			 ctl->card->card, ctl->elem);