Skip to content

Commit d906abe

Browse files
michaelrj-googlemichaelrj-google
committed
[libc][NFC] clean internal fd handling
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 5a6a4b6 commit d906abe

File tree

14 files changed

+198
-82
lines changed

14 files changed

+198
-82
lines changed

‎libc/docs/configure.rst

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ to learn about the defaults for your platform and target.
2929
- ``LIBC_CONF_ENABLE_STRONG_STACK_PROTECTOR``: Enable -fstack-protector-strong to defend against stack smashing attack.
3030
- ``LIBC_CONF_KEEP_FRAME_POINTER``: Keep frame pointer in functions for better debugging experience.
3131
* **"errno" options**
32-
- ``LIBC_CONF_ERRNO_MODE``: The implementation used for errno, acceptable values are LIBC_ERRNO_MODE_DEFAULT, LIBC_ERRNO_MODE_UNDEFINED, LIBC_ERRNO_MODE_THREAD_LOCAL, LIBC_ERRNO_MODE_SHARED, LIBC_ERRNO_MODE_EXTERNAL, and LIBC_ERRNO_MODE_SYSTEM.
32+
- ``LIBC_CONF_ERRNO_MODE``: The implementation used for errno, acceptable values are LIBC_ERRNO_MODE_DEFAULT, LIBC_ERRNO_MODE_UNDEFINED, LIBC_ERRNO_MODE_THREAD_LOCAL, LIBC_ERRNO_MODE_SHARED, LIBC_ERRNO_MODE_EXTERNAL, LIBC_ERRNO_MODE_SYSTEM, and LIBC_ERRNO_MODE_SYSTEM_INLINE.
3333
* **"general" options**
3434
- ``LIBC_ADD_NULL_CHECKS``: Add nullptr checks in the library's implementations to some functions for which passing nullptr is undefined behavior.
3535
* **"math" options**

‎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

0 commit comments

Comments
 (0)