Re: [LTP] [PATCH v5] Migrating the libhugetlbfs/testcases/truncate_sigbus_versus_oom.c test

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

Hi Samir,

Thanks for the migration. This is just a first line review on the major
issues related to this patch. Probably more will come with time if these
issues won't be achieved.

> Subject: [PATCH] Migrating the
>  libhugetlbfs/testcases/truncate_sigbus_versus_oom.c test

The subject should use imperative mood, e.g.:

  hugemmap37: Migrate truncate_sigbus_versus_oom from libhugetlbfs

> In this test case, we are verifying the bug fix commit that is attached as a part of the test case structure,
>
> Some kernel have a bug in the positioning of the test against

"Some kernel have" -> "Some kernels have". The first sentence ends with
a dangling comma and is vague. Consider dropping it entirely — the
second paragraph already explains the test clearly.

[...]

> +void setup(void)
> +{
> +	struct sigaction sa;

Two issues here:

1) setup() and cleanup() are missing the 'static' keyword.

2) 'sa' is uninitialized — sa_mask and other fields contain garbage.
   Zero-initialize it:

       struct sigaction sa = {};
       sigemptyset(&sa.sa_mask);

> +	sa.sa_flags = SA_SIGINFO;
> +	sa.sa_handler = sigbus_handler;

SA_SIGINFO tells the kernel to use sa_sigaction (3-argument handler),
but you assign to sa_handler (1-argument). This is undefined behavior
per POSIX. Since sigbus_handler() takes a single int, drop SA_SIGINFO:

    sa.sa_flags = 0;
    sa.sa_handler = sigbus_handler;

> +	totpages = SAFE_READ_MEMINFO(MEMINFO_HPAGE_FREE);
> +	hpage_size = tst_get_hugepage_size();

The totpages assignment here is dead code — it gets overwritten in
run_test() before use. Remove it from setup().

[...]

> +	fd = tst_creat_unlinked(MNTPOINT, 0, 0600);
> +	p = SAFE_MMAP(NULL, hpage_size, PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0);

[...]

> +	fdx = tst_creat_unlinked(MNTPOINT, 0, 0600);

[...]

> +	q = SAFE_MMAP(NULL, totpages * hpage_size, PROT_READ | PROT_WRITE, MAP_SHARED,
> +			fdx, 0);

fd and fdx are opened each iteration but only closed in cleanup(). On
-i N runs the previous fds leak. Similarly, p and q are mmap'd but
never munmapped, leaking virtual memory each iteration.

Close the fds and munmap both regions at the end of run_test(), resetting
fd/fdx to -1.

[...]

> +void cleanup(void)
> +{
> +	if (fd > 0)
> +		SAFE_CLOSE(fd);
> +	if (fdx > 0)
> +		SAFE_CLOSE(fdx);

if (fd != -1) and fd should be initialized to -1. zero is a valid value.

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

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