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

[wang lian <lianux.mm@gmail.com>]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=yes, Size: 1852 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.

Thanks a lot for pointing this out — it's a very reasonable concern.

Fortunately, this situation is handled by FIXTURE_TEARDOWN_PARENT, 
which reliably takes care of cleanup when the test exits early via ASSERT_* or SKIP(). 

During earlier testing (in v3), I did run into an issue where a missing cleanup 
led to run_vmtests hanging,which prompted me to make sure that subsequent versions 
correctly rely on the fixture teardown mechanism for child process termination.

So while your concern definitely makes sense, this specific case should be 
well-covered by the existing teardown logic.

Thanks again for the careful review!

Best regards,
wang lian