Re: [LTP] open01: fix cleanup file descriptor check

[linuxtestproject.agent@gmail.com]

Hi Jinseok,

On 2026-07-29, Jinseok Kim wrote:
> open01: fix cleanup file descriptor check

--- [PATCH 1/3] ---

> File descriptor 0 is valid, so checking fd > 0 may skip closing an
> successfully opened file.

Typo: "an successfully" -> "a successfully".

--- [PATCH 2/3] ---

>  } tcases[] = {
>  	{TEST_FILE, O_RDWR | O_CREAT, 01444, S_ISVTX, "sticky bit"},
> -	{TEST_DIR, O_DIRECTORY, 0, S_IFDIR, "directory bit"}
>  };

Only one entry is left in tcases[] now.

Is the .test + .tcnt parametrization still worth keeping here? The
convention is .test_all with a plain run() when there is a single test
case, unless more cases are planned on top of this.

>  	SAFE_CLOSE(fd);
>  	if (S_ISREG(buf.st_mode))
>  		SAFE_UNLINK(tc->filename);

This guard existed only to skip the unlink for the O_DIRECTORY case.

With that case removed the only remaining file is a regular one, so the
condition is always true. Can it be dropped and the unlink made
unconditional?

>   * 1. Open a new file with O_CREAT, fstat.st_mode should not have the
>   *    01000 (S_ISVTX) bit on. In Linux, the save text bit is *NOT* cleared.
> - * 2. Open a new directory with O_DIRECTORY, fstat.st_mode should have the
> - *    040000 (S_IFDIR) bit on.
>   */

The numbered list has a single "1." item left. Plain prose would read
better in the test catalog.

--- [PATCH 3/3] ---

> + * Verify that :manpage:`fstat(2)` correctly identifies various
> + * file types.

"various file types" does not say which ones. The commit message already
enumerates them, so how about moving that into the description, e.g.:

  Verify that :manpage:`fstat(2)` reports the correct file type in
  st_mode for regular files, directories, FIFOs, character devices and
  block devices.

> +#include <sys/sysmacros.h>
> +#include <fcntl.h>
> +
> +#include "tst_test.h"

O_PATH is used below but only <fcntl.h> is included. LTP carries a
fallback definition in include/lapi/fcntl.h for headers that predate
O_PATH, so "lapi/fcntl.h" should be included as well.

> +	TST_EXP_EXPR((buf.st_mode & S_IFMT) == tc->exp_type, "checking %s", tc->path);

On failure this only prints the path, not what was expected. Something
like "fstat() reports the expected type for %s" carries more information.

Also, for the character and block device cases only S_IFMT is checked
while st_rdev is ignored. Would it be worth comparing it against
makedev(1, 3) / makedev(7, 3) as well? statx01.c checks major/minor for
its device file.

> +static void cleanup(void)
> +{
> +	if (!access(REG_FILE, F_OK))
> +		SAFE_UNLINK(REG_FILE);
> +
> +	if (!access(DIR_FILE,  F_OK))
> +		SAFE_RMDIR(DIR_FILE);
> +
> +	if (!access(FIFO_FILE, F_OK))
> +		SAFE_UNLINK(FIFO_FILE);
> +
> +	if (!access(CHR_DEV, F_OK))
> +		SAFE_UNLINK(CHR_DEV);
> +
> +	if (!access(BLK_DEV, F_OK))
> +		SAFE_UNLINK(BLK_DEV);
> +}

Is any of this needed? Everything is created inside the framework tmpdir,
which is removed recursively at the end of the test, and MOUNT_PATH is
either a plain directory in that tmpdir or a tmpfs the framework unmounts
itself (prepare_and_mount_dev_fs() in lib/tst_test.c). Nothing here
outlives the test.

open11.c does the same touch/mkdir/mknod setup with .needs_devfs and has
no cleanup() at all.

Minor: "access(DIR_FILE,  F_OK)" has a double space.

> +	.mntpoint = MOUNT_PATH,
> +	.needs_devfs = 1,
> +	.needs_tmpdir = 1,
> +	.needs_root = 1,

.needs_tmpdir is redundant here, .mntpoint already implies the tmpdir
(see needs_tmpdir() in lib/tst_test.c). open11.c and fsetxattr02.c set
only .needs_devfs and .mntpoint.

Verdict - Needs revision

Pre-existing issues:

open01.c includes <errno.h> but never uses errno.

---
Note:

The agent can sometimes produce false positives although often its
findings are genuine. If you find issues with the review, please
comment this email or ignore the suggestions.

Regards,
LTP AI Reviewer

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