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;
Date:   Mon, 16 May 2022 16:02:30 +0100
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
Message-ID: <YoJnhulbKk49rZsw@sirena.org.uk>
References: <8598037d-0e24-9bc1-3f2c-a2751ec8e871@gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
        protocol="application/pgp-signature"; boundary="cH4kqHwkCNRl3JMX"
Content-Disposition: inline
In-Reply-To: <8598037d-0e24-9bc1-3f2c-a2751ec8e871@gmail.com>
Precedence: bulk


--cH4kqHwkCNRl3JMX
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

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. =20
> +		 * independent of any operation by software.
>  		 */

This should definitely be a separate commit.

>  		bool is_volatile =3D 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?

--cH4kqHwkCNRl3JMX
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQEzBAABCgAdFiEEreZoqmdXGLWf4p/qJNaLcl1Uh9AFAmKCZ4UACgkQJNaLcl1U
h9CEbgf9Eps4RQf5GwgggebMWvlsEDS4AHT1wMUSZOns3d44DpU8Em6Fg9BHGF19
Y8qy5envpJ30adNWBZNS+hH/Cr1mzHsP4CXYxADNBTMLDvwHAT7v0xDThc5lGNhi
TjMNxnYssD4uk5vwqEDjfukpqQ8ee2U5Mteysg3nNfvUK0Y4Ty7rLU11wHPdpsib
iuwVlm56PbJwhJz/++DDj5JfAQKz+7K5ZMWmTYNyf4flwB2UyWdueeKknrNh7HrK
etvB6eYnOddvoP1fLzsZxFWE6Kamk2Fb7UPfdmc9HX26XhvrSpQlre5e+Otbs9Ft
cLadxKCsaQ6kn9eKdRiwUQwfDNvXEg==
=9XeY
-----END PGP SIGNATURE-----

--cH4kqHwkCNRl3JMX--