-
Notifications
You must be signed in to change notification settings - Fork 13.6k
[libc] Implement fcntl() function #89507
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@llvm/pr-subscribers-libc Author: Vinayak Dev (vinayakdsci) ChangesImplements the Full diff: https://github.com/llvm/llvm-project/pull/89507.diff 9 Files Affected:
diff --git a/libc/config/linux/aarch64/entrypoints.txt b/libc/config/linux/aarch64/entrypoints.txt
index 1ac6bd93000082..5606b06967d629 100644
--- a/libc/config/linux/aarch64/entrypoints.txt
+++ b/libc/config/linux/aarch64/entrypoints.txt
@@ -22,6 +22,7 @@ set(TARGET_LIBC_ENTRYPOINTS
# fcntl.h entrypoints
libc.src.fcntl.creat
+ libc.src.fcntl.fcntl
libc.src.fcntl.open
libc.src.fcntl.openat
diff --git a/libc/config/linux/riscv/entrypoints.txt b/libc/config/linux/riscv/entrypoints.txt
index 87e82e5eb9a067..1caba997742b8a 100644
--- a/libc/config/linux/riscv/entrypoints.txt
+++ b/libc/config/linux/riscv/entrypoints.txt
@@ -22,6 +22,7 @@ set(TARGET_LIBC_ENTRYPOINTS
# fcntl.h entrypoints
libc.src.fcntl.creat
+ libc.src.fcntl.fcntl
libc.src.fcntl.open
libc.src.fcntl.openat
diff --git a/libc/config/linux/x86_64/entrypoints.txt b/libc/config/linux/x86_64/entrypoints.txt
index 2d8136536b218b..aec1bd4d5911ba 100644
--- a/libc/config/linux/x86_64/entrypoints.txt
+++ b/libc/config/linux/x86_64/entrypoints.txt
@@ -22,6 +22,7 @@ set(TARGET_LIBC_ENTRYPOINTS
# fcntl.h entrypoints
libc.src.fcntl.creat
+ libc.src.fcntl.fcntl
libc.src.fcntl.open
libc.src.fcntl.openat
diff --git a/libc/src/fcntl/CMakeLists.txt b/libc/src/fcntl/CMakeLists.txt
index 0b9ee47c4f7c13..77400e9050d08b 100644
--- a/libc/src/fcntl/CMakeLists.txt
+++ b/libc/src/fcntl/CMakeLists.txt
@@ -9,6 +9,13 @@ add_entrypoint_object(
.${LIBC_TARGET_OS}.creat
)
+add_entrypoint_object(
+ fcntl
+ ALIAS
+ DEPENDS
+ .${LIBC_TARGET_OS}.fcntl
+)
+
add_entrypoint_object(
open
ALIAS
diff --git a/libc/src/fcntl/fcntl.h b/libc/src/fcntl/fcntl.h
new file mode 100644
index 00000000000000..821e498f767fdd
--- /dev/null
+++ b/libc/src/fcntl/fcntl.h
@@ -0,0 +1,20 @@
+//===-- Implementation header of fcntl --------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_LIBC_SRC_FCNTL_FCNTL_H
+#define LLVM_LIBC_SRC_FCNTL_FCNTL_H
+
+#include <fcntl.h>
+
+namespace LIBC_NAMESPACE {
+
+int fcntl(int fd, int cmd, ...);
+
+} // namespace LIBC_NAMESPACE
+
+#endif // LLVM_LIBC_SRC_FCNTL_FCNTL_H
diff --git a/libc/src/fcntl/linux/CMakeLists.txt b/libc/src/fcntl/linux/CMakeLists.txt
index 87b8d4695c4fc5..f12d57a6b40a7b 100644
--- a/libc/src/fcntl/linux/CMakeLists.txt
+++ b/libc/src/fcntl/linux/CMakeLists.txt
@@ -10,6 +10,18 @@ add_entrypoint_object(
libc.src.errno.errno
)
+add_entrypoint_object(
+ fcntl
+ SRCS
+ fcntl.cpp
+ HDRS
+ ../fcntl.h
+ DEPENDS
+ libc.include.fcntl
+ libc.src.__support.OSUtil.osutil
+ libc.src.errno.errno
+)
+
add_entrypoint_object(
open
SRCS
diff --git a/libc/src/fcntl/linux/fcntl.cpp b/libc/src/fcntl/linux/fcntl.cpp
new file mode 100644
index 00000000000000..12376a518bdfa3
--- /dev/null
+++ b/libc/src/fcntl/linux/fcntl.cpp
@@ -0,0 +1,96 @@
+//===-- Implementation of fcntl -------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "src/fcntl/fcntl.h"
+
+#include "src/__support/OSUtil/syscall.h" // For internal syscall function.
+#include "src/__support/common.h"
+#include "src/errno/libc_errno.h"
+
+#include <stdarg.h>
+#include <sys/syscall.h> // For syscall numbers.
+
+namespace {
+struct fowner_ex {
+ int type;
+ pid_t pid;
+};
+} // namespace
+
+// The OFD file locks require special handling for LARGEFILES
+namespace LIBC_NAMESPACE {
+LLVM_LIBC_FUNCTION(int, fcntl, (int fd, int cmd, ...)) {
+ void *arg;
+ va_list varargs;
+ va_start(varargs, cmd);
+ arg = va_arg(varargs, void *);
+ va_end(varargs);
+
+ switch (cmd) {
+ case F_SETLKW:
+ return syscall_impl<int>(SYS_fcntl, fd, cmd, arg);
+ case F_OFD_SETLKW: {
+ struct flock *flk = (struct flock *)arg;
+ // convert the struct to a flock64
+ struct flock64 flk64;
+ flk64.l_type = flk->l_type;
+ flk64.l_whence = flk->l_whence;
+ flk64.l_start = flk->l_start;
+ flk64.l_len = flk->l_len;
+ flk64.l_pid = flk->l_pid;
+ // create a syscall
+ return syscall_impl<int>(SYS_fcntl, fd, cmd, &flk64);
+ }
+ case F_OFD_GETLK:
+ case F_OFD_SETLK: {
+ struct flock *flk = (struct flock *)arg;
+ // convert the struct to a flock64
+ struct flock64 flk64;
+ flk64.l_type = flk->l_type;
+ flk64.l_whence = flk->l_whence;
+ flk64.l_start = flk->l_start;
+ flk64.l_len = flk->l_len;
+ flk64.l_pid = flk->l_pid;
+ // create a syscall
+ int retVal = syscall_impl<int>(SYS_fcntl, fd, cmd, &flk64);
+ // On failure, return
+ if (retVal == -1)
+ return -1;
+ // Check for overflow, i.e. the offsets are not the same when cast
+ // to off_t from off64_t.
+ if ((off_t)flk64.l_len != flk64.l_len ||
+ (off_t)flk64.l_start != flk64.l_start) {
+ libc_errno = EOVERFLOW;
+ return -1;
+ }
+ // Now copy back into flk, in case flk64 got modified
+ flk->l_type = flk64.l_type;
+ flk->l_whence = flk64.l_whence;
+ flk->l_start = flk64.l_start;
+ flk->l_len = flk64.l_len;
+ flk->l_pid = flk64.l_pid;
+ return retVal;
+ }
+ // The general case
+ default: {
+ if (cmd == F_GETOWN) {
+ struct fowner_ex fex;
+ int retVal = syscall_impl<int>(SYS_fcntl, fd, F_GETOWN_EX, &fex);
+ if (retVal == -EINVAL)
+ return syscall_impl<int>(SYS_fcntl, fd, cmd, (void *)arg);
+ if ((uint64_t)retVal <= -4096UL)
+ return fex.type == F_OWNER_PGRP ? -fex.pid : fex.pid;
+
+ libc_errno = -retVal;
+ return -1;
+ }
+ return syscall_impl<int>(SYS_fcntl, fd, cmd, (void *)arg);
+ }
+ }
+}
+} // namespace LIBC_NAMESPACE
diff --git a/libc/test/src/fcntl/CMakeLists.txt b/libc/test/src/fcntl/CMakeLists.txt
index ae39d8d5f878c5..1c99009d5a6d4e 100644
--- a/libc/test/src/fcntl/CMakeLists.txt
+++ b/libc/test/src/fcntl/CMakeLists.txt
@@ -17,6 +17,21 @@ add_libc_unittest(
libc.test.UnitTest.ErrnoSetterMatcher
)
+add_libc_unittest(
+ fcntl_test
+ SUITE
+ libc_fcntl_unittests
+ SRCS
+ fcntl_test.cpp
+ DEPENDS
+ libc.include.fcntl
+ libc.src.errno.errno
+ libc.src.fcntl.fcntl
+ libc.src.fcntl.open
+ libc.src.unistd.close
+ libc.test.UnitTest.ErrnoSetterMatcher
+)
+
add_libc_unittest(
openat_test
SUITE
diff --git a/libc/test/src/fcntl/fcntl_test.cpp b/libc/test/src/fcntl/fcntl_test.cpp
new file mode 100644
index 00000000000000..81a7feaf151cf8
--- /dev/null
+++ b/libc/test/src/fcntl/fcntl_test.cpp
@@ -0,0 +1,146 @@
+//===-- Unittest for fcntl ------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "src/errno/libc_errno.h"
+#include "src/fcntl/fcntl.h"
+#include "src/fcntl/open.h"
+#include "src/unistd/close.h"
+#include "test/UnitTest/ErrnoSetterMatcher.h"
+#include "test/UnitTest/Test.h"
+
+#include <sys/stat.h>
+
+TEST(LlvmLibcFcntlTest, FcntlDupfd) {
+ using LIBC_NAMESPACE::testing::ErrnoSetterMatcher::Succeeds;
+ constexpr const char *TEST_FILE = "testdata/fcntl.test";
+ int fd2, fd3;
+ int fd = LIBC_NAMESPACE::open(TEST_FILE, O_CREAT | O_TRUNC, S_IRWXU);
+ ASSERT_ERRNO_SUCCESS();
+ ASSERT_GT(fd, 0);
+
+ fd2 = LIBC_NAMESPACE::fcntl(fd, F_DUPFD, 0);
+ ASSERT_ERRNO_SUCCESS();
+ ASSERT_GT(fd2, 0);
+
+ fd3 = LIBC_NAMESPACE::fcntl(fd, F_DUPFD, 10);
+ ASSERT_ERRNO_SUCCESS();
+ ASSERT_GT(fd3, 0);
+
+ ASSERT_THAT(LIBC_NAMESPACE::close(fd), Succeeds(0));
+ ASSERT_THAT(LIBC_NAMESPACE::close(fd2), Succeeds(0));
+ ASSERT_THAT(LIBC_NAMESPACE::close(fd3), Succeeds(0));
+}
+
+TEST(LlvmLibcFcntlTest, FcntlGetFl) {
+ using LIBC_NAMESPACE::testing::ErrnoSetterMatcher::Succeeds;
+ constexpr const char *TEST_FILE = "testdata/fcntl.test";
+ int retVal;
+ int fd = LIBC_NAMESPACE::open(TEST_FILE, O_CREAT | O_TRUNC, S_IRWXU);
+ ASSERT_ERRNO_SUCCESS();
+ ASSERT_GT(fd, 0);
+
+ retVal = LIBC_NAMESPACE::fcntl(fd, F_GETFL);
+ ASSERT_ERRNO_SUCCESS();
+ ASSERT_GT(retVal, -1);
+
+ ASSERT_THAT(LIBC_NAMESPACE::close(fd), Succeeds(0));
+}
+
+TEST(LlvmLibcFcntlTest, FcntlSetFl) {
+ using LIBC_NAMESPACE::testing::ErrnoSetterMatcher::Succeeds;
+ constexpr const char *TEST_FILE = "testdata/fcntl.test";
+
+ int retVal;
+ int fd = LIBC_NAMESPACE::open(TEST_FILE, O_CREAT | O_TRUNC | O_RDWR, S_IRWXU);
+ ASSERT_ERRNO_SUCCESS();
+ ASSERT_GT(fd, 0);
+
+ retVal = LIBC_NAMESPACE::fcntl(fd, F_GETFL);
+ ASSERT_ERRNO_SUCCESS();
+ ASSERT_GT(retVal, -1);
+
+ int oldFlags = LIBC_NAMESPACE::fcntl(fd, F_GETFL, 0);
+ ASSERT_ERRNO_SUCCESS();
+ ASSERT_GT(oldFlags, 0);
+
+ // Add the APPEND flag;
+ oldFlags |= O_APPEND;
+
+ retVal = LIBC_NAMESPACE::fcntl(fd, F_SETFL, oldFlags);
+ ASSERT_ERRNO_SUCCESS();
+ ASSERT_GT(retVal, -1);
+
+ // Remove the APPEND flag;
+ oldFlags = -oldFlags & O_APPEND;
+
+ retVal = LIBC_NAMESPACE::fcntl(fd, F_SETFL, oldFlags);
+ ASSERT_ERRNO_SUCCESS();
+ ASSERT_GT(retVal, -1);
+
+ ASSERT_THAT(LIBC_NAMESPACE::close(fd), Succeeds(0));
+}
+
+TEST(LlvmLibcFcntlTest, FcntlGetLkRead) {
+ using LIBC_NAMESPACE::testing::ErrnoSetterMatcher::Succeeds;
+ constexpr const char *TEST_FILE = "testdata/fcntl.test";
+
+ struct flock flk, svflk;
+ int retVal;
+ int fd = LIBC_NAMESPACE::open(TEST_FILE, O_CREAT | O_TRUNC | O_RDONLY);
+ ASSERT_ERRNO_SUCCESS();
+ ASSERT_GT(fd, 0);
+
+ flk.l_type = F_RDLCK;
+ flk.l_start = 0;
+ flk.l_whence = SEEK_SET;
+ flk.l_len = 50;
+
+ // copy flk into svflk
+ svflk = flk;
+
+ retVal = LIBC_NAMESPACE::fcntl(fd, F_GETLK, &svflk);
+ ASSERT_ERRNO_SUCCESS();
+ ASSERT_GT(retVal, -1);
+ ASSERT_NE((int)flk.l_type, F_WRLCK); // File should not be write locked.
+
+ retVal = LIBC_NAMESPACE::fcntl(fd, F_SETLK, &svflk);
+ ASSERT_ERRNO_SUCCESS();
+ ASSERT_GT(retVal, -1);
+
+ ASSERT_THAT(LIBC_NAMESPACE::close(fd), Succeeds(0));
+}
+
+TEST(LlvmLibcFcntlTest, FcntlGetLkWrite) {
+ using LIBC_NAMESPACE::testing::ErrnoSetterMatcher::Succeeds;
+ constexpr const char *TEST_FILE = "testdata/fcntl.test";
+
+ struct flock flk, svflk;
+ int retVal;
+ int fd = LIBC_NAMESPACE::open(TEST_FILE, O_CREAT | O_TRUNC | O_RDWR, S_IRWXU);
+ ASSERT_ERRNO_SUCCESS();
+ ASSERT_GT(fd, 0);
+
+ flk.l_type = F_WRLCK;
+ flk.l_start = 0;
+ flk.l_whence = SEEK_SET;
+ flk.l_len = 0;
+
+ // copy flk into svflk
+ svflk = flk;
+
+ retVal = LIBC_NAMESPACE::fcntl(fd, F_GETLK, &svflk);
+ ASSERT_ERRNO_SUCCESS();
+ ASSERT_GT(retVal, -1);
+ ASSERT_NE((int)flk.l_type, F_RDLCK); // File should not be read locked.
+
+ retVal = LIBC_NAMESPACE::fcntl(fd, F_SETLK, &svflk);
+ ASSERT_ERRNO_SUCCESS();
+ ASSERT_GT(retVal, -1);
+
+ ASSERT_THAT(LIBC_NAMESPACE::close(fd), Succeeds(0));
+}
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This patch is a good start, but it needs some more work to be complete.
For this to work in fullbuild mode you need to add the function to spec/posix.td. The rest of the changes should be covered by the comments, but feel free to reach out if you need help with any of this.
libc/src/fcntl/fcntl.h
Outdated
#ifndef LLVM_LIBC_SRC_FCNTL_FCNTL_H | ||
#define LLVM_LIBC_SRC_FCNTL_FCNTL_H | ||
|
||
#include <fcntl.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this include isn't necessary here since the function prototype doesn't use anything from the public API.
libc/src/fcntl/linux/fcntl.cpp
Outdated
case F_SETLKW: | ||
return syscall_impl<int>(SYS_fcntl, fd, cmd, arg); | ||
case F_OFD_SETLKW: { | ||
struct flock *flk = (struct flock *)arg; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be a reinterpret_cast
libc/src/fcntl/linux/fcntl.cpp
Outdated
return -1; | ||
// Check for overflow, i.e. the offsets are not the same when cast | ||
// to off_t from off64_t. | ||
if ((off_t)flk64.l_len != flk64.l_len || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use static_cast
instead of C style casts here (and below)
libc/test/src/fcntl/fcntl_test.cpp
Outdated
#include "test/UnitTest/ErrnoSetterMatcher.h" | ||
#include "test/UnitTest/Test.h" | ||
|
||
#include <sys/stat.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this include here?
be20343
to
7aeef13
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the new version, this is much closer to being finished. There are a couple of minor things to fix but I think after those are done it should be complete.
libc/src/fcntl/linux/fcntl.cpp
Outdated
F_OWNER_PID, | ||
F_OWNER_PGRP, | ||
}; | ||
struct fowner_ex { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you add a comment explaining where this is normally defined? I don't see any documentation on it in posix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The struct is specific to linux -> https://man7.org/linux/man-pages/man2/fcntl.2.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, in that case you should add a type header for it, similar to the others.
libc/src/fcntl/linux/fcntl.cpp
Outdated
} | ||
// The general case | ||
default: { | ||
if (cmd == F_GETOWN) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is F_GETOWN
in the default case and not with the rest of the switch
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry about this, must have missed my scrutiny. Will fix.
libc/src/fcntl/linux/fcntl.cpp
Outdated
if (retVal == -EINVAL) | ||
return syscall_impl<int>(SYS_fcntl, fd, cmd, | ||
reinterpret_cast<void *>(arg)); | ||
if (static_cast<uint64_t>(retVal) <= -4096UL) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is retVal
being cast to an unsigned value before comparing against a negative number? It seems like using int64_t
on both sides would be better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check allows a general check against syscall errors, and also because MAX_ERRNO is defined to be 4095 in the kernel. Checking against -4096UL means that if the value is strictly larger than this, than it is certain that the result is malformed. Note that -4096UL would mean 2^64 - 4096, by using two's complement here, which would have not been preserved in an int.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This simply increases the range of the possible results from [0..(2^63)] to [0..2^64-4096].
libc/test/src/fcntl/fcntl_test.cpp
Outdated
|
||
TEST(LlvmLibcFcntlTest, FcntlDupfd) { | ||
using LIBC_NAMESPACE::testing::ErrnoSetterMatcher::Succeeds; | ||
constexpr const char *TEST_FILE = "testdata/fcntl.test"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For these tests you should use the libc_make_test_file_path
function to ensure that the files end up in the correct directory. Look at test/src/unistd/read_write_test.cpp
for an example.
Also, each test should use a different filename since they may be run in parallel.
7aeef13
to
4b5c842
Compare
34262f3
to
ee519c2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This patch is fine for overlay mode but I found some issues with the test when trying it in fullbuild mode.
d14d873
to
e1aa582
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests pass on my machine in both overlay and fullbuild modes. LGTM. Ping me when you want me to merge this for you.
@michaelrj-google Thanks for the approval! It would be great if you could merge it on my behalf. |
Looks like this broke something. With this I'm getting a compile time error when running See the log here: https://hastebin.skyra.pw/uhuroxeyab.bash Reverting aca5117 fixed this for me. I'm on debian 12 x86_64 |
Hi @HendrikHuebner , do you have development headers for I am saying this because the |
Fixes #84968.
Implements the
fcntl()
function defined in thefcntl.h
header.