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

From: Mark Brown <broonie@kernel.org>
To: Siddh Raman Pant <siddhpant.gh@gmail.com>
Cc: Jaroslav Kysela <perex@perex.cz>, Takashi Iwai <tiwai@suse.com>,
	Shuah Khan <skhan@linuxfoundation.org>,
	alsa-devel@alsa-project.org, linux-kselftest@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH] selftests: alsa: Better error messages
Date: Mon, 16 May 2022 16:02:30 +0100	[thread overview]
Message-ID: <YoJnhulbKk49rZsw@sirena.org.uk> (raw)
In-Reply-To: <8598037d-0e24-9bc1-3f2c-a2751ec8e871@gmail.com>

[-- Attachment #1: Type: text/plain, Size: 2207 bytes --]

On Fri, May 13, 2022 at 07:10:57PM +0530, Siddh Raman Pant wrote:

> This allows for potentially better machine-parsing due to an
> expected / fixed format. Also because of eyecandy reasons.

As I said in reply to Takashi's mail I'm not convinced about all the
changes in here, a lot of it's really bikesheddy at the best of times
and to be honest there's more here that I don't like than do.  The
changes aren't entirely consistent in the final style either so
presumably not great if there is any machine parsing going on.  It'd be
much better to split this up into separate commits for separate changes,
that'd be a lot easier to review if nothing else.

>  	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?

> -				ksft_print_msg("%s getting info for %d\n",
> -					       snd_strerror(err),
> -					       ctl_data->name);
> +				ksft_print_msg("%s : %s while getting info\n",
> +					       ctl_data->name, snd_strerror(err));

Why add the space before the : here?  That really is not idiomatic for
Unix stuff, or just natural language.

> @@ -542,11 +541,12 @@ static bool show_mismatch(struct ctl_data *ctl, int index,
>  		/*
>  		 * NOTE: The volatile attribute means that the hardware
>  		 * can voluntarily change the state of control element
> -		 * independent of any operation by software.  
> +		 * independent of any operation by software.
>  		 */

This should definitely be a separate commit.

>  		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?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]