Re: [LTP] [PATCH] [PATCH] [V2] Migrating the libhugetlbfs/testcases/ptrace-write-hugepage.c

[Andrea Cervesato via ltp <ltp@lists.linux.it>]

Hi Pavithra,

Thanks for the migration. A few things to address below.

First, the commit subject should follow the LTP convention, e.g.:

  hugemmap41: Migrate ptrace-write-hugepage from libhugetlbfs

The body "Changed testcase to use new API" is too brief. Please explain
what the original test does and why it is being migrated (preserving
regression coverage for the ptrace hugepage fix, etc.).

> +/*\
> + * [Description]
> + *

The [Description] tag is deprecated and must not be used. Just remove
the line and keep the description text directly after the /*\ opener.

> +static volatile int ready_to_trace;

[...]

> +	ready_to_trace = 1;

[...]

> +	while (!ready_to_trace)
> +		;

This busy-wait spin loop combined with a SIGCHLD handler is fragile and
wastes CPU. The whole signal handler + spin loop can be replaced with
TST_PROCESS_STATE_WAIT() after PTRACE_ATTACH:

    err = ptrace(PTRACE_ATTACH, cpid, NULL, NULL);
    if (err)
        tst_brk(TFAIL | TERRNO, "ptrace(ATTACH) failed");

    TST_PROCESS_STATE_WAIT(cpid, 't');

This waits for the child to enter tracing stop, which is exactly what
the original code was trying to achieve. It also eliminates the need for
sigchld_handler() and the ready_to_trace variable entirely.

> +static void run_test(void)
> +{

[...]

> +	hpage_size = tst_get_hugepage_size();
> +	fd = tst_creat_unlinked(MNTPOINT, 0, 0600);

Both hpage_size and fd are static variables set inside run_test(). When
run with -i N, fd is overwritten each iteration without closing the
previous one, leaking file descriptors. Move these to a setup() callback
so they are initialized once, or close fd at the end of each iteration.

Also, ready_to_trace is never reset to 0 at the top of run_test(), so
on -i N runs the spin loop is skipped on iteration 2+, causing a race.
(This goes away if you adopt TST_PROCESS_STATE_WAIT() as suggested
above.)

> +	SAFE_PIPE(pipefd);

[...]

> +	SAFE_READ(1, pipefd[0], &p, sizeof(p));

The pipe file descriptors are never closed. Close pipefd[1] in the
parent after fork, and close pipefd[0] after the read. In the child,
close pipefd[0] before use and close pipefd[1] after the write.

> +static void child(int hugefd, int pipefd)
> +{
> +	void *p;
> +
> +	p = SAFE_MMAP(NULL, hpage_size, PROT_READ|PROT_WRITE, MAP_SHARED,
> +		      hugefd, 0);

[...]

> +	pause();
> +}

The child maps a hugepage but never calls SAFE_MUNMAP(). Add a munmap
before pause(), or restructure so the mapping is cleaned up.

> +	int signal;

[...]

> +	SAFE_WAITPID(cpid, &signal, 0);

Minor: the variable name "signal" shadows the signal() function. Rename
it to "status" for clarity.

Regards,
--
Andrea Cervesato
SUSE QE Automation Engineer Linux
andrea.cervesato@suse.com

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp