Re: [LTP] [PATCH] [PATCH V6] syscalls: ipc: Add shmget hugepage test

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

Hi Pavithra,

A few issues to address before this can be merged.

> Changes in v6:
> - Added "tst_test.h" and "tst_hugepage.h" instead of hugetlb.h
> - Modified code to use TEST() macro

The commit body should explain *why* the test is being added — what
behavior it validates or what gap it fills. Changelog entries belong
below the --- separator in the patch email, not in the commit message
body. Please write a proper description here.

> + * [Descripiton]

Two problems: "Descripiton" is a typo, and the [Description] label
itself is deprecated in LTP doc comments. Drop the label entirely and
write the description text directly.

> +static int raw_fd;

raw_fd is zero-initialized by the compiler but fd 0 is stdin. More
importantly, raw_fd is opened in run_test() and never closed anywhere —
not at the end of run_test(), not in cleanup(). This leaks a file
descriptor per test invocation.

Initialize it to -1, close it at the end of run_test(), and guard it
in cleanup():

  static int raw_fd = -1;

  /* in cleanup(): */
  if (raw_fd != -1)
      SAFE_CLOSE(raw_fd);

> +static size_t size;
> +static size_t i;

These are only used inside run_test() and do not carry state between
calls. `i` should be declared as local and `size` can be initialized
inside the setup() function.

> +	shmid = SAFE_SHMGET(IPC_PRIVATE, size, SHM_HUGETLB|SHM_R|SHM_W);
> +
> +	shmaddr = SAFE_SHMAT(shmid, 0, SHM_RND);
> +	tst_res(TINFO, "shmaddr: %p\n", shmaddr);

Two issues here:

1. tst_res() appends its own newline — the "\n" in the format string
   produces a spurious blank line in the output. Remove it.

2. shmaddr is not tracked in cleanup(). If run_test() is aborted after
   SAFE_SHMAT (e.g. by a tst_brk() from a future change or a signal),
   the segment remains attached. Add to cleanup():

     if (shmaddr)
         SAFE_SHMDT(shmaddr);

   And reset shmaddr = NULL after SAFE_SHMDT in run_test().

> +	SAFE_SHMDT(shmaddr);
> +}

When the test is run with -i N, each call to run_test() overwrites
shmid and raw_fd with fresh values without releasing the previous
iteration's resources:

  - The previous raw_fd is never closed   -> fd leak per iteration.
  - The previous shmid is never IPC_RMID'd before being overwritten
    -> shm segment leak per iteration; cleanup() only removes the last.

Remove shmid at the end of run_test() and reset it to -1 there, so
cleanup() is only a safety net. Close raw_fd at the end of run_test()
as well.

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

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