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] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 16 May 2022 16:02:30 +0100
From:   Mark Brown <broonie@...nel.org>
To:     Siddh Raman Pant <siddhpant.gh@...il.com>
Cc:     Jaroslav Kysela <perex@...ex.cz>, Takashi Iwai <tiwai@...e.com>,
        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

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?

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)