Re: [PATCH] selftests: alsa: Better error messages

[Siddh Raman Pant <siddhpant.gh@gmail.com>]

Thank you very much, Takashi, and Mark, for reviewing the patch. Helps me getting
the hang of kernel development coding styles and conventions while starting out.

The particular motivation for this was that this test tends to potentially
generate a very long list of warnings/errors.

On Mon, May 16, 2022 At 20:32:30 +0530, Mark Brown wrote:
>>  	if (err < 0) {
>> -		ksft_print_msg("Unable to parse custom alsa-lib configuration: %s\n",
>> +		ksft_print_msg("Unable to parse custom alsa-lib configuration (%s)\n",
>>  			       snd_strerror(err));
> 
> I'm really unconvinced that replacing : with () is helping either people
> or machines - the form we have at the minute is probably more common for
> command line tools?

The intent was to separate card and error with the colon. While it may not affect
parsing, you are right, the colon separator is seemingly the standard. Apologies.

> Why add the space before the : here?  That really is not idiomatic for
> Unix stuff, or just natural language.
> ...
> This should definitely be a separate commit.

You are right. Again, apologies for this.

>>  		bool is_volatile = snd_ctl_elem_info_is_volatile(ctl->info);
>> -		ksft_print_msg("%s.%d expected %lld but read %lld, is_volatile %d\n",
>> -			       ctl->name, index, expected_int, read_int, is_volatile);
>> +		ksft_print_msg("%s.%d : Expected %lld, but read %lld (%s)\n",
>> +			       ctl->name, index, expected_int, read_int,
>> +			       (is_volatile ? "Volatile" : "Non-volatile"));
> 
> I don't understand the comma here?

Those are independent clauses, hence used a comma. Looking back, the "but" can probably
be removed here for brevity.


Please let me know if there are any other things which bugs you, and whether or not
should I send a v2 with the issues addressed.

Thanks for the reviews,
Siddh