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

[linuxtestproject.agent@gmail.com]

On Thu, Jun 04, 2026 at 12:24:10 +0530, Sachin Sant wrote:
> fs/acl: Add ACL_USER_OBJ permissions test
> fs/acl: Add ACL mask interaction tests
> fs/acl: Add ACL_OTHER permissions test
> fs/acl: Add default ACL inheritance test
> fs/acl: Add chmod/chown ACL interaction tests
> fs/acl: Add symlink ACL operations test
> fs/acl: Add extended attributes test
> fs/acl: Remove old shell-based ACL test

Hi Sachin,

Thanks for the patch series. The overall direction is good — replacing the
old bash-based tacl_xattr.sh with modular C tests is the right move. I have
a few issues below that need addressing before this can be merged.

---

[PATCH 1/8] fs/acl: Add ACL_USER_OBJ permissions test (acl_user_obj01.c)

The run() function contains two independent test cases — deny-by-mode and
grant-by-ACL — both calling tst_res(TPASS/TFAIL,...) and with the second
depending on no shared mutable state from the first. Using .test_all for
two distinct cases means a failure of the first silently skips the second,
and both results appear under the same test counter. Change this to:

    .test  = run,
    .tcnt  = 2,

and dispatch on n inside run():

    static void run(unsigned int n)
    {
        switch (n) {
        case 0: test_deny_by_mode(); break;
        case 1: test_grant_by_acl();  break;
        }
    }

This matches the pattern used in acl_mask01.c and acl_file_ops01.c.

---

[PATCH 6/8] fs/acl: Add symlink ACL operations test (acl_link01.c)

The test verifies that acl_get_file() via a symlink returns the same result
as acl_get_file() on the target file directly. It does NOT verify that the
ACL was actually set to the intended value.

Consider the failure mode: if acl_set_file(TESTSYMLINK, ...) silently acted
on the symlink's own metadata rather than following it to the target, the
call would still return 0 (or EOPNOTSUPP), then both acl_get_file(TESTFILE)
and acl_get_file(TESTSYMLINK) would still agree with each other (both
reflecting the original 0600 permission), and the test would TPASS even
though the ACL was never applied to the target.

The commit message and the TPASS string both claim "ACL set via symlink was
applied to target file (rwxrw----)" but the code never checks that the
actual ACL content is rwxrw----.

After the set-via-symlink step, read back the ACL from the target file
directly and verify it matches the expected entries
(ACL_USER_OBJ=rwx, ACL_GROUP_OBJ=rw, ACL_OTHER=---) before proceeding to
the symlink-vs-direct comparison.

---

[PATCH 7/8] fs/acl: Add extended attributes test (xattr_test01.c)

Two issues:

1. test_xattr_backup_restore() does not test backup/restore

   The backup step hard-codes the attribute values into the file using the
   same compile-time constants (XATTR_TEST1_VALUE, XATTR_TEST2_VALUE) that
   were used to set the xattrs:

       fprintf(fp, "%s=\"%s\"\n", XATTR_TEST1_NAME, XATTR_TEST1_VALUE);
       fprintf(fp, "%s=\"%s\"\n", XATTR_TEST2_NAME, XATTR_TEST2_VALUE);

   This means the backup file is written from constants, not read from the
   filesystem. The test exercises the restore-and-verify path, but not the
   backup path (reading live xattrs from the filesystem and saving them).
   A genuine backup/restore test must generate the backup by reading the
   attributes off the file (e.g., via getxattr/listxattr) and saving the
   actual runtime values. As written, the test would pass even if the
   previous setxattr calls had silently written wrong values or nothing at
   all, because the backup never actually reads from the filesystem state.

   Either rename the test to reflect what it actually tests
   ("xattr_set_restore" or similar) or fix the backup step to genuinely
   read live xattr values before removing them.

2. Mixed use of TST_EXP_PASS / TST_EXP_PASS_SILENT inflates the TPASS count

   test_xattr() on a clean run emits five TPASS results:
     - TST_EXP_POSITIVE(getxattr TESTDIR)       → TPASS
     - TST_EXP_POSITIVE(getxattr TESTFILE)      → TPASS
     - TST_EXP_PASS(removexattr)                → TPASS
     - TST_EXP_FAIL(getxattr after removal)     → TPASS
     - tst_res(TPASS, "Extended attributes...") → TPASS

   Likewise test_xattr_backup_restore() emits four. Each function is run
   as a single test slot (.tcnt = 2), so each slot should produce exactly
   one TPASS on success. Use TST_EXP_PASS_SILENT (and
   TST_EXP_POSITIVE_SILENT / manual errno checks) for all intermediate
   operations, and emit one tst_res(TPASS, ...) per function at the end.
   Or conversely, remove the final tst_res(TPASS, ...) and keep only the
   individual macro results — but then be consistent throughout.

---

Minor nits (not blocking, fix at discretion):

* acl_lib.h: The extern declarations for user1_uid, user2_uid, etc. must
  be matched by a definition in every translation unit that actually
  references them. Tests that don't use the user infrastructure
  (acl_link01.c, xattr_test01.c) include the header but provide no
  definition. This happens to be harmless today because none of the
  static-inline helpers that use those variables are instantiated in those
  binaries. However it is fragile: adding a single call to any user-aware
  helper in those files would cause a link error. Consider moving the
  globals (and their initializers) to an acl_lib.c file so the header
  only needs the extern declarations, or at minimum add a comment
  documenting the requirement.

* acl_lib.h: cleanup_test_paths() unconditionally tries to unlink
  XATTR_BACKUP_FILE. Tests that never create that file (acl_link01.c,
  acl_mask01.c, etc.) silently get an ENOENT, which is harmless, but the
  cleanup function is carrying responsibility for a resource it cannot
  know was created. Consider splitting out cleanup_xattr_backup() or
  handling the backup file inside xattr_test01.c's own cleanup.

Thanks,
Copilot

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