Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <3f2e9cb6-9b3a-5054-34a8-7c7e1c77a15a@gmail.com>
Date:   Tue, 17 May 2022 00:20:01 +0530
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.8.1
Subject: Re: [PATCH] selftests: alsa: Better error messages
Content-Language: en-US
To:     Mark Brown <broonie@kernel.org>, Takashi Iwai <tiwai@suse.com>
Cc:     Jaroslav Kysela <perex@perex.cz>,
        Shuah Khan <skhan@linuxfoundation.org>,
        alsa-devel@alsa-project.org, linux-kselftest@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <8598037d-0e24-9bc1-3f2c-a2751ec8e871@gmail.com>
 <YoJnhulbKk49rZsw@sirena.org.uk>
From:   Siddh Raman Pant <siddhpant.gh@gmail.com>
In-Reply-To: <YoJnhulbKk49rZsw@sirena.org.uk>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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