linux-kernel - Re: [PATCH] selftests: alsa: Better error messages

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <3f2e9cb6-9b3a-5054-34a8-7c7e1c77a15a@gmail.com>
Date:   Tue, 17 May 2022 00:20:01 +0530
From:   Siddh Raman Pant <siddhpant.gh@...il.com>
To:     Mark Brown <broonie@...nel.org>, Takashi Iwai <tiwai@...e.com>
Cc:     Jaroslav Kysela <perex@...ex.cz>,
        Shuah Khan <skhan@...uxfoundation.org>,
        alsa-devel@...a-project.org, linux-kselftest@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] selftests: alsa: Better error messages

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