Skip to content

Commit 51689c9

Browse files
michaelrj-googlemichaelrj-google
authored
[libc][NFC] clean internal fd handling (#143991)
The previous internal fcntl implementation modified errno directly, this patch fixes that. This patch also moves open and close into OSUtil since they are used in multiple places. There are more places that need similar cleanup but only got comments in this patch to keep it relatively reviewable. Related to: #143937
1 parent ec330cf commit 51689c9

File tree

13 files changed

+197
-81
lines changed

13 files changed

+197
-81
lines changed

‎libc/src/__support/File/linux/file.cpp‎

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -19,8 +19,8 @@
1919
#include "src/__support/macros/config.h"
2020

2121
#include "hdr/fcntl_macros.h" // For mode_t and other flags to the open syscall
22-
#include <sys/stat.h> // For S_IS*, S_IF*, and S_IR* flags.
23-
#include <sys/syscall.h> // For syscall numbers
22+
#include <sys/stat.h> // For S_IS*, S_IF*, and S_IR* flags.
23+
#include <sys/syscall.h> // For syscall numbers
2424

2525
namespace LIBC_NAMESPACE_DECL {
2626

@@ -128,10 +128,11 @@ ErrorOr<LinuxFile *> create_file_from_fd(int fd, const char *mode) {
128128
return Error(EINVAL);
129129
}
130130

131-
int fd_flags = internal::fcntl(fd, F_GETFL);
132-
if (fd_flags == -1) {
131+
auto result = internal::fcntl(fd, F_GETFL);
132+
if (!result.has_value()) {
133133
return Error(EBADF);
134134
}
135+
int fd_flags = result.value();
135136

136137
using OpenMode = File::OpenMode;
137138
if (((fd_flags & O_ACCMODE) == O_RDONLY &&
@@ -145,8 +146,9 @@ ErrorOr<LinuxFile *> create_file_from_fd(int fd, const char *mode) {
145146
if ((modeflags & static_cast<ModeFlags>(OpenMode::APPEND)) &&
146147
!(fd_flags & O_APPEND)) {
147148
do_seek = true;
148-
if (internal::fcntl(fd, F_SETFL,
149-
reinterpret_cast<void *>(fd_flags | O_APPEND)) == -1) {
149+
if (!internal::fcntl(fd, F_SETFL,
150+
reinterpret_cast<void *>(fd_flags | O_APPEND))
151+
.has_value()) {
150152
return Error(EBADF);
151153
}
152154
}

‎libc/src/__support/OSUtil/fcntl.h‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,12 +8,18 @@
88
#ifndef LLVM_LIBC_SRC___SUPPORT_OSUTIL_FCNTL_H
99
#define LLVM_LIBC_SRC___SUPPORT_OSUTIL_FCNTL_H
1010

11+
#include "hdr/types/mode_t.h"
12+
#include "src/__support/error_or.h"
1113
#include "src/__support/macros/config.h"
1214

1315
namespace LIBC_NAMESPACE_DECL {
1416
namespace internal {
1517

16-
int fcntl(int fd, int cmd, void *arg = nullptr);
18+
ErrorOr<int> fcntl(int fd, int cmd, void *arg = nullptr);
19+
20+
ErrorOr<int> open(const char *path, int flags, mode_t mode_flags = 0);
21+
22+
ErrorOr<int> close(int fd);
1723

1824
} // namespace internal
1925
} // namespace LIBC_NAMESPACE_DECL

‎libc/src/__support/OSUtil/linux/CMakeLists.txt‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,6 @@ add_object_library(
1616
.${LIBC_TARGET_ARCHITECTURE}.linux_${LIBC_TARGET_ARCHITECTURE}_util
1717
libc.src.__support.common
1818
libc.src.__support.CPP.string_view
19-
libc.src.errno.errno
2019
libc.hdr.fcntl_macros
2120
libc.hdr.types.struct_flock
2221
libc.hdr.types.struct_flock64

‎libc/src/__support/OSUtil/linux/fcntl.cpp‎

Lines changed: 53 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -8,23 +8,24 @@
88

99
#include "src/__support/OSUtil/fcntl.h"
1010

11+
#include "hdr/errno_macros.h"
1112
#include "hdr/fcntl_macros.h"
13+
#include "hdr/types/mode_t.h"
1214
#include "hdr/types/off_t.h"
1315
#include "hdr/types/struct_f_owner_ex.h"
1416
#include "hdr/types/struct_flock.h"
1517
#include "hdr/types/struct_flock64.h"
1618
#include "src/__support/OSUtil/syscall.h" // For internal syscall function.
1719
#include "src/__support/common.h"
18-
#include "src/__support/libc_errno.h"
20+
#include "src/__support/error_or.h"
1921
#include "src/__support/macros/config.h"
2022

21-
#include <stdarg.h>
2223
#include <sys/syscall.h> // For syscall numbers.
2324

2425
namespace LIBC_NAMESPACE_DECL {
2526
namespace internal {
2627

27-
int fcntl(int fd, int cmd, void *arg) {
28+
ErrorOr<int> fcntl(int fd, int cmd, void *arg) {
2829
#if SYS_fcntl
2930
constexpr auto FCNTL_SYSCALL_ID = SYS_fcntl;
3031
#elif defined(SYS_fcntl64)
@@ -33,8 +34,7 @@ int fcntl(int fd, int cmd, void *arg) {
3334
#error "fcntl and fcntl64 syscalls not available."
3435
#endif
3536

36-
int new_cmd = cmd;
37-
switch (new_cmd) {
37+
switch (cmd) {
3838
case F_OFD_SETLKW: {
3939
struct flock *flk = reinterpret_cast<struct flock *>(arg);
4040
// convert the struct to a flock64
@@ -45,8 +45,11 @@ int fcntl(int fd, int cmd, void *arg) {
4545
flk64.l_len = flk->l_len;
4646
flk64.l_pid = flk->l_pid;
4747
// create a syscall
48-
return LIBC_NAMESPACE::syscall_impl<int>(FCNTL_SYSCALL_ID, fd, new_cmd,
49-
&flk64);
48+
int ret =
49+
LIBC_NAMESPACE::syscall_impl<int>(FCNTL_SYSCALL_ID, fd, cmd, &flk64);
50+
if (ret < 0)
51+
return Error(-ret);
52+
return ret;
5053
}
5154
case F_OFD_GETLK:
5255
case F_OFD_SETLK: {
@@ -59,60 +62,80 @@ int fcntl(int fd, int cmd, void *arg) {
5962
flk64.l_len = flk->l_len;
6063
flk64.l_pid = flk->l_pid;
6164
// create a syscall
62-
int retVal = LIBC_NAMESPACE::syscall_impl<int>(FCNTL_SYSCALL_ID, fd,
63-
new_cmd, &flk64);
65+
int ret =
66+
LIBC_NAMESPACE::syscall_impl<int>(FCNTL_SYSCALL_ID, fd, cmd, &flk64);
6467
// On failure, return
65-
if (retVal == -1)
66-
return -1;
68+
if (ret < 0)
69+
return Error(-1);
6770
// Check for overflow, i.e. the offsets are not the same when cast
6871
// to off_t from off64_t.
6972
if (static_cast<off_t>(flk64.l_len) != flk64.l_len ||
70-
static_cast<off_t>(flk64.l_start) != flk64.l_start) {
71-
libc_errno = EOVERFLOW;
72-
return -1;
73-
}
73+
static_cast<off_t>(flk64.l_start) != flk64.l_start)
74+
return Error(EOVERFLOW);
75+
7476
// Now copy back into flk, in case flk64 got modified
7577
flk->l_type = flk64.l_type;
7678
flk->l_whence = flk64.l_whence;
7779
flk->l_start = static_cast<decltype(flk->l_start)>(flk64.l_start);
7880
flk->l_len = static_cast<decltype(flk->l_len)>(flk64.l_len);
7981
flk->l_pid = flk64.l_pid;
80-
return retVal;
82+
return ret;
8183
}
8284
case F_GETOWN: {
8385
struct f_owner_ex fex;
8486
int ret = LIBC_NAMESPACE::syscall_impl<int>(FCNTL_SYSCALL_ID, fd,
8587
F_GETOWN_EX, &fex);
86-
if (ret >= 0)
87-
return fex.type == F_OWNER_PGRP ? -fex.pid : fex.pid;
88-
libc_errno = -ret;
89-
return -1;
88+
if (ret < 0)
89+
return Error(-ret);
90+
return fex.type == F_OWNER_PGRP ? -fex.pid : fex.pid;
9091
}
9192
#ifdef SYS_fcntl64
9293
case F_GETLK: {
9394
if constexpr (FCNTL_SYSCALL_ID == SYS_fcntl64)
94-
new_cmd = F_GETLK64;
95+
cmd = F_GETLK64;
9596
break;
9697
}
9798
case F_SETLK: {
9899
if constexpr (FCNTL_SYSCALL_ID == SYS_fcntl64)
99-
new_cmd = F_SETLK64;
100+
cmd = F_SETLK64;
100101
break;
101102
}
102103
case F_SETLKW: {
103104
if constexpr (FCNTL_SYSCALL_ID == SYS_fcntl64)
104-
new_cmd = F_SETLKW64;
105+
cmd = F_SETLKW64;
105106
break;
106107
}
107108
#endif
108109
}
109-
int retVal = LIBC_NAMESPACE::syscall_impl<int>(FCNTL_SYSCALL_ID, fd, new_cmd,
110-
reinterpret_cast<void *>(arg));
111-
if (retVal >= 0) {
112-
return retVal;
113-
}
114-
libc_errno = -retVal;
115-
return -1;
110+
111+
// default, but may use rewritten cmd from above.
112+
int ret = LIBC_NAMESPACE::syscall_impl<int>(FCNTL_SYSCALL_ID, fd, cmd,
113+
reinterpret_cast<void *>(arg));
114+
if (ret < 0)
115+
return Error(-ret);
116+
return ret;
117+
}
118+
119+
ErrorOr<int> open(const char *path, int flags, mode_t mode_flags) {
120+
#ifdef SYS_open
121+
int fd = LIBC_NAMESPACE::syscall_impl<int>(SYS_open, path, flags, mode_flags);
122+
#else
123+
int fd = LIBC_NAMESPACE::syscall_impl<int>(SYS_openat, AT_FDCWD, path, flags,
124+
mode_flags);
125+
#endif
126+
if (fd < 0)
127+
return Error(-fd);
128+
129+
return fd;
130+
}
131+
132+
ErrorOr<int> close(int fd) {
133+
int ret = LIBC_NAMESPACE::syscall_impl<int>(SYS_close, fd);
134+
135+
if (ret < 0)
136+
return Error(-ret);
137+
138+
return ret;
116139
}
117140

118141
} // namespace internal

‎libc/src/fcntl/linux/CMakeLists.txt‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ add_entrypoint_object(
1919
DEPENDS
2020
libc.hdr.fcntl_macros
2121
libc.src.__support.OSUtil.osutil
22+
libc.src.errno.errno
2223
)
2324

2425
add_entrypoint_object(

‎libc/src/fcntl/linux/fcntl.cpp‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010

1111
#include "src/__support/OSUtil/fcntl.h"
1212
#include "src/__support/common.h"
13+
#include "src/__support/libc_errno.h"
1314
#include "src/__support/macros/config.h"
1415

1516
#include <stdarg.h>
@@ -22,7 +23,14 @@ LLVM_LIBC_FUNCTION(int, fcntl, (int fd, int cmd, ...)) {
2223
va_start(varargs, cmd);
2324
arg = va_arg(varargs, void *);
2425
va_end(varargs);
25-
return LIBC_NAMESPACE::internal::fcntl(fd, cmd, arg);
26+
27+
auto result = LIBC_NAMESPACE::internal::fcntl(fd, cmd, arg);
28+
29+
if (!result.has_value()) {
30+
libc_errno = result.error();
31+
return -1;
32+
}
33+
return result.value();
2634
}
2735

2836
} // namespace LIBC_NAMESPACE_DECL

‎libc/src/fcntl/linux/open.cpp‎

Lines changed: 9 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -8,15 +8,13 @@
88

99
#include "src/fcntl/open.h"
1010

11-
#include "src/__support/OSUtil/syscall.h" // For internal syscall function.
11+
#include "hdr/fcntl_macros.h"
12+
#include "hdr/types/mode_t.h"
13+
#include "src/__support/OSUtil/fcntl.h"
1214
#include "src/__support/common.h"
1315
#include "src/__support/libc_errno.h"
1416
#include "src/__support/macros/config.h"
15-
16-
#include "hdr/fcntl_macros.h"
17-
#include "hdr/types/mode_t.h"
1817
#include <stdarg.h>
19-
#include <sys/syscall.h> // For syscall numbers.
2018

2119
namespace LIBC_NAMESPACE_DECL {
2220

@@ -31,17 +29,13 @@ LLVM_LIBC_FUNCTION(int, open, (const char *path, int flags, ...)) {
3129
va_end(varargs);
3230
}
3331

34-
#ifdef SYS_open
35-
int fd = LIBC_NAMESPACE::syscall_impl<int>(SYS_open, path, flags, mode_flags);
36-
#else
37-
int fd = LIBC_NAMESPACE::syscall_impl<int>(SYS_openat, AT_FDCWD, path, flags,
38-
mode_flags);
39-
#endif
40-
if (fd > 0)
41-
return fd;
32+
auto result = internal::open(path, flags, mode_flags);
4233

43-
libc_errno = -fd;
44-
return -1;
34+
if (!result.has_value()) {
35+
libc_errno = result.error();
36+
return -1;
37+
}
38+
return result.value();
4539
}
4640

4741
} // namespace LIBC_NAMESPACE_DECL

‎libc/src/sys/auxv/linux/getauxval.cpp‎

Lines changed: 26 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,8 @@
88

99
#include "src/sys/auxv/getauxval.h"
1010
#include "config/app.h"
11+
#include "hdr/fcntl_macros.h"
12+
#include "src/__support/OSUtil/fcntl.h"
1113
#include "src/__support/common.h"
1214
#include "src/__support/libc_errno.h"
1315
#include "src/__support/macros/config.h"
@@ -17,14 +19,18 @@
1719
#include "src/__support/threads/callonce.h"
1820
#include "src/__support/threads/linux/futex_word.h"
1921

22+
// -----------------------------------------------------------------------------
23+
// TODO: This file should not include other public libc functions. Calling other
24+
// public libc functions is an antipattern within LLVM-libc. This needs to be
25+
// cleaned up. DO NOT COPY THIS.
26+
// -----------------------------------------------------------------------------
27+
2028
// for mallocing the global auxv
2129
#include "src/sys/mman/mmap.h"
2230
#include "src/sys/mman/munmap.h"
2331

2432
// for reading /proc/self/auxv
25-
#include "src/fcntl/open.h"
2633
#include "src/sys/prctl/prctl.h"
27-
#include "src/unistd/close.h"
2834
#include "src/unistd/read.h"
2935

3036
// getauxval will work either with or without __cxa_atexit support.
@@ -60,17 +66,18 @@ class AuxvMMapGuard {
6066
constexpr static size_t AUXV_MMAP_SIZE = sizeof(AuxEntry) * MAX_AUXV_ENTRIES;
6167

6268
AuxvMMapGuard()
63-
: ptr(mmap(nullptr, AUXV_MMAP_SIZE, PROT_READ | PROT_WRITE,
64-
MAP_PRIVATE | MAP_ANONYMOUS, -1, 0)) {}
69+
: ptr(LIBC_NAMESPACE::mmap(nullptr, AUXV_MMAP_SIZE,
70+
PROT_READ | PROT_WRITE,
71+
MAP_PRIVATE | MAP_ANONYMOUS, -1, 0)) {}
6572
~AuxvMMapGuard() {
6673
if (ptr != MAP_FAILED)
67-
munmap(ptr, AUXV_MMAP_SIZE);
74+
LIBC_NAMESPACE::munmap(ptr, AUXV_MMAP_SIZE);
6875
}
6976
void submit_to_global() {
7077
// atexit may fail, we do not set it to global in that case.
7178
int ret = __cxa_atexit(
7279
[](void *) {
73-
munmap(auxv, AUXV_MMAP_SIZE);
80+
LIBC_NAMESPACE::munmap(auxv, AUXV_MMAP_SIZE);
7481
auxv = nullptr;
7582
},
7683
nullptr, nullptr);
@@ -90,10 +97,16 @@ class AuxvMMapGuard {
9097

9198
class AuxvFdGuard {
9299
public:
93-
AuxvFdGuard() : fd(open("/proc/self/auxv", O_RDONLY | O_CLOEXEC)) {}
100+
AuxvFdGuard() {
101+
auto result = internal::open("/proc/self/auxv", O_RDONLY | O_CLOEXEC);
102+
if (!result.has_value())
103+
fd = -1;
104+
105+
fd = result.value();
106+
}
94107
~AuxvFdGuard() {
95108
if (fd != -1)
96-
close(fd);
109+
internal::close(fd);
97110
}
98111
bool valid() const { return fd != -1; }
99112
int get() const { return fd; }
@@ -135,7 +148,8 @@ static void initialize_auxv_once(void) {
135148
bool error_detected = false;
136149
// Read until we use up all the available space or we finish reading the file.
137150
while (available_size != 0) {
138-
ssize_t bytes_read = read(fd_guard.get(), buf, available_size);
151+
ssize_t bytes_read =
152+
LIBC_NAMESPACE::read(fd_guard.get(), buf, available_size);
139153
if (bytes_read <= 0) {
140154
if (libc_errno == EINTR)
141155
continue;
@@ -158,7 +172,7 @@ static AuxEntry read_entry(int fd) {
158172
size_t size = sizeof(AuxEntry);
159173
char *ptr = reinterpret_cast<char *>(&buf);
160174
while (size > 0) {
161-
ssize_t ret = read(fd, ptr, size);
175+
ssize_t ret = LIBC_NAMESPACE::read(fd, ptr, size);
162176
if (ret < 0) {
163177
if (libc_errno == EINTR)
164178
continue;
@@ -195,7 +209,8 @@ LLVM_LIBC_FUNCTION(unsigned long, getauxval, (unsigned long id)) {
195209
return search_auxv(app.auxv_ptr, id);
196210

197211
static FutexWordType once_flag;
198-
callonce(reinterpret_cast<CallOnceFlag *>(&once_flag), initialize_auxv_once);
212+
LIBC_NAMESPACE::callonce(reinterpret_cast<CallOnceFlag *>(&once_flag),
213+
initialize_auxv_once);
199214
if (auxv != nullptr)
200215
return search_auxv(auxv, id);
201216

0 commit comments

Comments
 (0)