Re: [PATCH v5] selftests/mm: add process_madvise() tests

[Mark Brown <broonie@kernel.org>]

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

On Mon, Jul 14, 2025 at 08:25:33PM +0800, wang lian wrote:

> +TEST_F(process_madvise, remote_collapse)
> +{

> +	self->child_pid = fork();
> +	ASSERT_NE(self->child_pid, -1);

> +	ret = read(pipe_info[0], &info, sizeof(info));
> +	if (ret <= 0) {
> +		waitpid(self->child_pid, NULL, 0);
> +		SKIP(return, "Failed to read child info from pipe.\n");
> +	}
> +	ASSERT_EQ(ret, sizeof(info));

> +cleanup:
> +	/* Cleanup */
> +	kill(self->child_pid, SIGKILL);
> +	waitpid(self->child_pid, NULL, 0);
> +	if (pidfd >= 0)
> +		close(pidfd);

The cleanup here won't get run if we skip or assert, skipping will
return immediately (you could replace the return with a 'goto cleanup')
and the asserts will exit the test immediately.  This will mean we leak
the child.  This isn't an issue for things that are memory mapped or
tracked with file descriptors, the harness will for a new child for each
test so anything that's cleaned up with the process will be handled, but
that doesn't apply to child processes.

I think doing the child setup in a fixture should DTRT but I haven't
gone through in full detail to verify that this is the case.

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