Re: [LTP] fs/acl: Add ACL_USER_OBJ permissions test

[linuxtestproject.agent@gmail.com]

Hi Sachin,

Thank you for this series. Converting the old monolithic tacl_xattr.sh to
modular C tests using the new LTP API is a clear improvement. The code
structure, helper library design, ACL logic, and test coverage are all solid.
I found two commit-message accuracy issues that need to be fixed before the
series can be applied.

---

[PATCH 7/8] fs/acl: Add extended attributes test

The commit subject and body both describe 6 test cases:

  Add xattr_test01 test with 6 test cases to validate extended
  attribute operations:

  1. Set/get xattr on directory
  2. Set/get xattr on regular file
  3. List xattrs on directory
  4. Remove xattr from file
  5. Backup and restore xattrs
  6. Verify xattr operations follow symlinks to target

But the actual implementation has only 2 sub-tests (.tcnt = 2):

  case 0: test_xattr()          — covers items 1, 2, 4
  case 1: test_xattr_backup_restore() — covers item 5

Items 3 ("List xattrs on directory") and 6 ("Verify xattr operations
follow symlinks to target") are described in the commit message but are
not implemented anywhere in xattr_test01.c. There is no listxattr(2) call
and no symlink creation or xattr-through-symlink test in the file.

The test's own description block is accurate:

  * This test validates:
  *  - Extended attributes set/get/remove operations
  *  - Extended attributes backup and restore operations

Please update the commit message to match the actual implementation.
Either drop the unimplemented items from the list (and say "2 test cases"),
or implement the missing cases.

---

[PATCH 8/8] fs/acl: Remove old shell-based ACL test

The replacement test summary in this commit is copied from (or refers to)
the inaccurate count above:

  - xattr_test01: Extended attributes (6 tests)

xattr_test01 has .tcnt = 2, not 6. Update this line to:

  - xattr_test01: Extended attributes (2 tests)

---

Verdict: Needs revision

---

Code review notes (no action required):

- acl_lib.h is clean: the static-inline helper design, SAFE_FORK/
  SAFE_WAITPID usage, setgroups(0,NULL)+setgid+setuid ordering, and
  _exit(errno) through WEXITSTATUS are all correct.

- All tst_test structs correctly set needs_root=1, mount_device=1,
  forks_child=1, and list appropriate ACL-capable filesystems. The
  needs_cmds guard for useradd/userdel is a nice touch.

- ACL semantics in every sub-test are correct: ACL_USER_OBJ is unaffected
  by ACL_MASK; ACL_OTHER is likewise mask-exempt; ACL_USER/ACL_GROUP/
  ACL_GROUP_OBJ are correctly restricted by the mask. The acl_inherit01
  umask=0 + mode=0666 → 0444 path matches POSIX ACL inheritance rules.

- The Makefile wildcard build and runtest/fs entries are all consistent.
  The old tacl_xattr.sh had no runtest entry, so no runtest removal is
  needed.

Thanks,
LTP AI Reviewer

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