On Tue, Jul 15, 2025 at 06:58:05PM +0800, wang lian wrote: > > On Mon, Jul 14, 2025 at 08:25:33PM +0800, wang lian wrote:
> > > + /* 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 > 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. Are you sure the parent cleanup sees variables set in the child and actually takes effect? We fork() the child so it should be a new VM which means that values stored there won't be seen by the parent, it'll see whatever values it had before the fork(). It does also seem like bad practice to have duplicated cleanup code in both the test and the fixture cleanups, the fixture cleanup always runs so we should just use that all the time if it's in use (that's the whole idea really). Indeed as things stand since the cleanup in the test doesn't reset self->child_pid so if the cleanup at the fixture level does anything there's a minor risk that we might race with the PID being reused and kill some new task The fixture teardown handler also doesn't do the close(pidfd), either that's redundant for the in test cleanup or should be in the fixture (given that it's a child process it should be redundant as any open file descriptors are closed when the process exits).
signature.asc
Description: PGP signature
