Re: [PATCH v1 2/2] tests/chamelium/audio: Fix resource leaks on assertion failure

[Sebastian Brzezinka <sebastian.brzezinka@intel.com>]

On Wed Mar 4, 2026 at 11:05 AM CET, Mohammed Bilal wrote:
> Fatal assertions can bypass cleanup paths, causing resources to remain
> unreleased when failures occur during audio tests.
>
> Replace fatal assertions with non-fatal checks and route failures
> through a common cleanup path to ensure proper cleanup.
>
> Signed-off-by: Mohammed Bilal <mohammed.bilal@intel.com>
> ---
>  tests/chamelium/kms_chamelium_audio.c | 43 ++++++++++++++++++++-------
>  1 file changed, 33 insertions(+), 10 deletions(-)
>
> diff --git a/tests/chamelium/kms_chamelium_audio.c b/tests/chamelium/kms_chamelium_audio.c
> index 2967e3c50..41cbe92d9 100644
> --- a/tests/chamelium/kms_chamelium_audio.c
> +++ b/tests/chamelium/kms_chamelium_audio.c
> @@ -426,9 +426,21 @@ static bool test_audio_frequencies(struct audio_state *state)
>  
>  	audio_state_start(state, "frequencies");
>  
> -	igt_assert_f(state->capture.rate == state->playback.rate,
> -		     "Capture rate (%dHz) doesn't match playback rate (%dHz)\n",
> -		     state->capture.rate, state->playback.rate);
> +	/* Initialize heap pointers to NULL so the cleanup path can safely
> +	 * free them regardless of where an error occurs.
> +	 */
> +	channel = NULL;
> +	buf = NULL;
> +	recv = NULL;
> +	recv_len = 0;
> +	success = false;
It feels a bit messy to assign NULL here, it's cleaner at
declaration. Plus, we don't really need the goto logic for an input
check, the original assert was enough since it's just verifying the
function's parameters. Ideally, this whole block should be moved to the
beginning of the function, before any init.

> +
> +	if (state->capture.rate != state->playback.rate) {
> +		igt_critical("Capture rate (%dHz) doesn't match playback "
> +			     "rate (%dHz)\n",
> +			     state->capture.rate, state->playback.rate);
> +		goto out;
> +	}
>  
>  	/* Needs to be a multiple of 128, because that's the number of samples
>  	 * we get per channel each time we receive an audio page from the
> @@ -447,10 +459,6 @@ static bool test_audio_frequencies(struct audio_state *state)
>  	buf = malloc(sizeof(int32_t) * buf_cap);
>  	buf_len = 0;
>  
> -	recv = NULL;
> -	recv_len = 0;
> -
> -	success = false;
>  	streak = 0;
>  	while (!success && state->msec < AUDIO_TIMEOUT) {
>  		audio_state_receive(state, &recv, &recv_len);
> @@ -460,13 +468,21 @@ static bool test_audio_frequencies(struct audio_state *state)
>  
>  		if (buf_len < buf_cap)
>  			continue;
> -		igt_assert(buf_len == buf_cap);
> +		if (buf_len != buf_cap) {
I see why you're using goto for the cleanup path, but let's tweak the
condition so it’s easier to read at a glance basically, failing
if buf_len exceeds buf_cap. We could also handle this by checking the
divisibility of recv_len and buf_cap instead.

-- 
Best regards,
Sebastian