Re: [LTP] syscalls/xattr: Add runtime probe for socket xattr backports

[linuxtestproject.agent@gmail.com]

Hi Darren,

Thank you for this patch. The approach is sound: a runtime probe for
backported socket xattr support is the right way to avoid false
failures on downstream kernels. However there are a few issues in
fsetxattr02.c that need fixing before this can be applied.

Verdict: Needs revision

---

[1] fsetxattr02.c — variable declared after executable statement

The new `int socket_idx = -1;` declaration appears after the
executable statement `socket_xattr_supported = tst_kvercmp(7, 1, 0)
>= 0;`. The existing setup() declares all its local variables at the
top of the function (`size_t i = 0;` etc.). This new variable must
follow the same convention.

Move `int socket_idx = -1;` to the top of setup(), alongside the
existing declarations:

  static void setup(void)
  {
  	size_t i = 0;
  	int socket_idx = -1;         /* add here */
  	struct sockaddr_un sun;
  	...

[2] fsetxattr02.c — indentation uses spaces instead of tabs

The `int socket_idx = -1;` line in the patch uses 4 spaces for
indentation instead of a tab character:

  -    int socket_idx = -1;       /* 4 spaces — wrong */
  +	int socket_idx = -1;       /* tab — correct */

The rest of the function uses tabs throughout.

[3] fsetxattr02.c — comment doesn't reference the upstream commit

The comment added to fsetxattr02.c is vague compared to the one added
to setxattr02.c, which names the upstream commit and its title:

  setxattr02.c (new):
    /*
     * Commit dc0876b9846d "xattr: support extended attributes on sockets"
     * merged in 7.1.0 mainline, but might be backported.
     * Try to set xattr on socket to detect support.
     */

  fsetxattr02.c (new):
    /*
     * Probe for backports on older kernels.
     * If the version is < 7.1.0 but fsetxattr() succeeds, we assume the patch
     * has been backported.
     */

These describe the same feature. The fsetxattr02 comment should
include the commit reference for consistency. Use the same wording as
in setxattr02.c.

[4] fsetxattr02.c — inline comment is unnecessary

  /* Clean up the probed xattr to avoid interfering with the actual test */
  SAFE_FREMOVEXATTR(tc[socket_idx].fd, tc[socket_idx].key);

The comment states the obvious. SAFE_FREMOVEXATTR after a successful
setxattr-probe speaks for itself. Drop it, matching the style of the
equivalent block in setxattr02.c which has no such comment.

---

Aside from those four points the logic is correct. In particular:

- setxattr02.c looks clean; no issues there.
- The probe correctly uses the already-opened socket fd in fsetxattr02,
  mirroring the path-based probe in setxattr02.
- Silently ignoring non-zero probe results is correct: EPERM means no
  backport, and EOPNOTSUPP is caught later in verify_fsetxattr().
- `socket_xattr_supported = true` (using bool) is consistent with the
  field type (`static bool socket_xattr_supported`).
- The commit message is accurate and the Signed-off-by is present.

---

LTP AI Reviewer

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