[libc][fcntl] Simplify the handling of the return value from syscall …

[simonzgx]

Fixes #95570

[llvmbot]

@llvm/pr-subscribers-libc

Author: Xu Zhang (simonzgx)

Changes

---

Full diff: https://github.com/llvm/llvm-project/pull/96325.diff

3 Files Affected:

• (modified) libc/src/__support/OSUtil/linux/fcntl.cpp (+3-7)
• (modified) libc/test/src/fcntl/CMakeLists.txt (+1)
• (modified) libc/test/src/fcntl/fcntl_test.cpp (+20)

diff --git a/libc/src/__support/OSUtil/linux/fcntl.cpp b/libc/src/__support/OSUtil/linux/fcntl.cpp
index b087f898c395d..083deb8c33681 100644
--- a/libc/src/__support/OSUtil/linux/fcntl.cpp
+++ b/libc/src/__support/OSUtil/linux/fcntl.cpp
@@ -67,15 +67,11 @@ int fcntl(int fd, int cmd, void *arg) {
   }
   case F_GETOWN: {
     struct f_owner_ex fex;
-    int retVal =
+    int ret =
         LIBC_NAMESPACE::syscall_impl<int>(SYS_fcntl, fd, F_GETOWN_EX, &fex);
-    if (retVal == -EINVAL)
-      return LIBC_NAMESPACE::syscall_impl<int>(SYS_fcntl, fd, cmd,
-                                               reinterpret_cast<void *>(arg));
-    if (static_cast<unsigned long>(retVal) <= -4096UL)
+    if (ret >= 0)
       return fex.type == F_OWNER_PGRP ? -fex.pid : fex.pid;
-
-    libc_errno = -retVal;
+    libc_errno = -ret;
     return -1;
   }
   // The general case
diff --git a/libc/test/src/fcntl/CMakeLists.txt b/libc/test/src/fcntl/CMakeLists.txt
index aae296f074bea..48048b7fe8866 100644
--- a/libc/test/src/fcntl/CMakeLists.txt
+++ b/libc/test/src/fcntl/CMakeLists.txt
@@ -29,6 +29,7 @@ add_libc_unittest(
     libc.src.fcntl.fcntl
     libc.src.fcntl.open
     libc.src.unistd.close
+    libc.src.unistd.getpid
     libc.hdr.types.struct_flock
     libc.hdr.fcntl_macros
     libc.test.UnitTest.ErrnoSetterMatcher
diff --git a/libc/test/src/fcntl/fcntl_test.cpp b/libc/test/src/fcntl/fcntl_test.cpp
index fc909acc1aa0b..f50109322e6b2 100644
--- a/libc/test/src/fcntl/fcntl_test.cpp
+++ b/libc/test/src/fcntl/fcntl_test.cpp
@@ -12,9 +12,11 @@
 #include "src/fcntl/fcntl.h"
 #include "src/fcntl/open.h"
 #include "src/unistd/close.h"
+#include "src/unistd/getpid.h"
 #include "test/UnitTest/ErrnoSetterMatcher.h"
 #include "test/UnitTest/Test.h"
 
+#include <fcntl.h>
 #include <stdio.h>
 #include <sys/stat.h> // For S_IRWXU
 
@@ -163,3 +165,21 @@ TEST(LlvmLibcFcntlTest, UseAfterClose) {
   ASSERT_EQ(-1, LIBC_NAMESPACE::fcntl(fd, F_GETFL));
   ASSERT_ERRNO_EQ(EBADF);
 }
+
+TEST(LlvmLibcFcntlTest, SetGetOwnerTest) {
+  using LIBC_NAMESPACE::testing::ErrnoSetterMatcher::Succeeds;
+  pid_t pid = LIBC_NAMESPACE::getpid();
+  ASSERT_GT(pid, -1);
+  constexpr const char *TEST_FILE_NAME = "testdata/fcntl_set_get_owner.test";
+  auto TEST_FILE = libc_make_test_file_path(TEST_FILE_NAME);
+  int fd = LIBC_NAMESPACE::open(TEST_FILE, O_CREAT | O_TRUNC | O_RDWR, S_IRWXU);
+  ASSERT_ERRNO_SUCCESS();
+  ASSERT_GT(fd, 0);
+  int ret = LIBC_NAMESPACE::fcntl(fd, F_SETOWN, pid);
+  ASSERT_ERRNO_SUCCESS();
+  ASSERT_GT(ret, -1);
+  int ret2 = LIBC_NAMESPACE::fcntl(fd, F_GETOWN);
+  ASSERT_ERRNO_SUCCESS();
+  ASSERT_EQ(ret2, pid);
+  ASSERT_THAT(LIBC_NAMESPACE::close(fd), Succeeds(0));
+}

[simonzgx]

I just fixed a UT failure. Please review it again and help merge if everything is OK.

[nickdesaulniers]

Why was the unit test failing? Why would clearing errno fix it?

[nickdesaulniers]

Suggested change

	LIBC_NAMESPACE::libc_errno = 0;
	// Clear errno for subsequent unit tests.
	LIBC_NAMESPACE::libc_errno = 0;

[nickdesaulniers]

I just noticed that other tests tend to zero libc_errno before running any operations. We should clear errno first, for tests that need it, rather than reset it after tests that touch it.

[simonzgx]

Okay, fixed. And I'm curious, is this just a matter of convention?

[nickdesaulniers]

Convention. I think UseAfterClose and the other unit tests that use ASSERT_ERRNO_ macros should be doing this, too.