From f8ddc3d8732feece71d6ee55cdfc4d61a5a7e16e Mon Sep 17 00:00:00 2001 From: LemonBoy Date: Sun, 22 Nov 2020 23:28:40 +0100 Subject: [PATCH 1/4] std: Fix file locking logic for BSD targets --- lib/std/child_process.zig | 1 + lib/std/fs.zig | 8 +++++--- lib/std/fs/test.zig | 12 ------------ lib/std/os.zig | 4 ++++ 4 files changed, 10 insertions(+), 15 deletions(-) diff --git a/lib/std/child_process.zig b/lib/std/child_process.zig index de9099076a..b61fe9470d 100644 --- a/lib/std/child_process.zig +++ b/lib/std/child_process.zig @@ -368,6 +368,7 @@ pub const ChildProcess = struct { error.DeviceBusy => unreachable, error.FileLocksNotSupported => unreachable, error.BadPathName => unreachable, // Windows-only + error.WouldBlock => unreachable, else => |e| return e, } else diff --git a/lib/std/fs.zig b/lib/std/fs.zig index db95b2a7d1..2419ea2a51 100644 --- a/lib/std/fs.zig +++ b/lib/std/fs.zig @@ -405,7 +405,7 @@ pub const Dir = struct { else => false, }; if (mem.eql(u8, name, ".") or mem.eql(u8, name, "..") or - (skip_zero_fileno and bsd_entry.d_fileno == 0)) + (skip_zero_fileno and bsd_entry.d_fileno == 0)) { continue :start_over; } @@ -863,8 +863,8 @@ pub const Dir = struct { 0; const lock_flag: u32 = if (has_flock_open_flags) switch (flags.lock) { .None => @as(u32, 0), - .Shared => os.O_SHLOCK, - .Exclusive => os.O_EXLOCK, + .Shared => os.O_SHLOCK | nonblocking_lock_flag, + .Exclusive => os.O_EXLOCK | nonblocking_lock_flag, } else 0; const O_LARGEFILE = if (@hasDecl(os, "O_LARGEFILE")) os.O_LARGEFILE else 0; @@ -1178,6 +1178,7 @@ pub const Dir = struct { error.NoSpaceLeft => unreachable, // not providing O_CREAT error.PathAlreadyExists => unreachable, // not providing O_CREAT error.FileLocksNotSupported => unreachable, // locking folders is not supported + error.WouldBlock => unreachable, // can't happen for directories else => |e| return e, }; return Dir{ .fd = fd }; @@ -1221,6 +1222,7 @@ pub const Dir = struct { error.NoSpaceLeft => unreachable, // not providing O_CREAT error.PathAlreadyExists => unreachable, // not providing O_CREAT error.FileLocksNotSupported => unreachable, // locking folders is not supported + error.WouldBlock => unreachable, // can't happen for directories else => |e| return e, }; return Dir{ .fd = fd }; diff --git a/lib/std/fs/test.zig b/lib/std/fs/test.zig index 5469192f6c..3850758baa 100644 --- a/lib/std/fs/test.zig +++ b/lib/std/fs/test.zig @@ -691,9 +691,6 @@ test "realpath" { test "open file with exclusive nonblocking lock twice" { if (builtin.os.tag == .wasi) return error.SkipZigTest; - // TODO: fix this test on FreeBSD. https://github.com/ziglang/zig/issues/1759 - if (builtin.os.tag == .freebsd) return error.SkipZigTest; - const filename = "file_nonblocking_lock_test.txt"; var tmp = tmpDir(.{}); @@ -709,9 +706,6 @@ test "open file with exclusive nonblocking lock twice" { test "open file with shared and exclusive nonblocking lock" { if (builtin.os.tag == .wasi) return error.SkipZigTest; - // TODO: fix this test on FreeBSD. https://github.com/ziglang/zig/issues/1759 - if (builtin.os.tag == .freebsd) return error.SkipZigTest; - const filename = "file_nonblocking_lock_test.txt"; var tmp = tmpDir(.{}); @@ -727,9 +721,6 @@ test "open file with shared and exclusive nonblocking lock" { test "open file with exclusive and shared nonblocking lock" { if (builtin.os.tag == .wasi) return error.SkipZigTest; - // TODO: fix this test on FreeBSD. https://github.com/ziglang/zig/issues/1759 - if (builtin.os.tag == .freebsd) return error.SkipZigTest; - const filename = "file_nonblocking_lock_test.txt"; var tmp = tmpDir(.{}); @@ -791,9 +782,6 @@ test "open file with exclusive lock twice, make sure it waits" { test "open file with exclusive nonblocking lock twice (absolute paths)" { if (builtin.os.tag == .wasi) return error.SkipZigTest; - // TODO: fix this test on FreeBSD. https://github.com/ziglang/zig/issues/1759 - if (builtin.os.tag == .freebsd) return error.SkipZigTest; - const allocator = testing.allocator; const file_paths: [1][]const u8 = .{"zig-test-absolute-paths.txt"}; diff --git a/lib/std/os.zig b/lib/std/os.zig index e7c618431c..c93f8d72a5 100644 --- a/lib/std/os.zig +++ b/lib/std/os.zig @@ -1020,6 +1020,8 @@ pub const OpenError = error{ BadPathName, InvalidUtf8, + + WouldBlock, } || UnexpectedError; /// Open and possibly create a file. Keeps trying if it gets interrupted. @@ -1201,6 +1203,7 @@ pub fn openatZ(dir_fd: fd_t, file_path: [*:0]const u8, flags: u32, mode: mode_t) EEXIST => return error.PathAlreadyExists, EBUSY => return error.DeviceBusy, EOPNOTSUPP => return error.FileLocksNotSupported, + EWOULDBLOCK => return error.WouldBlock, else => |err| return unexpectedErrno(err), } } @@ -4187,6 +4190,7 @@ pub fn realpathZ(pathname: [*:0]const u8, out_buffer: *[MAX_PATH_BYTES]u8) RealP const flags = if (builtin.os.tag == .linux) O_PATH | O_NONBLOCK | O_CLOEXEC else O_NONBLOCK | O_CLOEXEC; const fd = openZ(pathname, flags, 0) catch |err| switch (err) { error.FileLocksNotSupported => unreachable, + error.WouldBlock => unreachable, else => |e| return e, }; defer close(fd); From 0d4f05bb8ad12e140b0f93157c3c3d2c87e2738c Mon Sep 17 00:00:00 2001 From: LemonBoy Date: Mon, 23 Nov 2020 09:08:55 +0100 Subject: [PATCH 2/4] std: Remove O_NONBLOCK flag after locking We only need O_NONBLOCK when O_SHLOCK/O_EXLOCK are used and we don't want open() to block, don't let this bit leak to the user fd. --- lib/std/fs.zig | 36 ++++++++++++++++++++++++++++++++++-- 1 file changed, 34 insertions(+), 2 deletions(-) diff --git a/lib/std/fs.zig b/lib/std/fs.zig index 2419ea2a51..f927d53962 100644 --- a/lib/std/fs.zig +++ b/lib/std/fs.zig @@ -733,10 +733,10 @@ pub const Dir = struct { // (Or if it's darwin, as darwin's `open` doesn't support the O_SYNC flag) const has_flock_open_flags = @hasDecl(os, "O_EXLOCK") and !is_darwin; if (has_flock_open_flags) { - const nonblocking_lock_flag = if (flags.lock_nonblocking) + const nonblocking_lock_flag: u32 = if (flags.lock_nonblocking) os.O_NONBLOCK | os.O_SYNC else - @as(u32, 0); + 0; os_flags |= switch (flags.lock) { .None => @as(u32, 0), .Shared => os.O_SHLOCK | nonblocking_lock_flag, @@ -771,6 +771,22 @@ pub const Dir = struct { }); } + if (has_flock_open_flags and flags.lock_nonblocking) { + var fl_flags = os.fcntl(fd, os.F_GETFL, 0) catch |err| switch (err) { + error.FileBusy => unreachable, + error.Locked => unreachable, + error.PermissionDenied => unreachable, + else => |e| return e, + }; + fl_flags &= ~@as(usize, os.O_NONBLOCK); + _ = os.fcntl(fd, os.F_SETFL, fl_flags) catch |err| switch (err) { + error.FileBusy => unreachable, + error.Locked => unreachable, + error.PermissionDenied => unreachable, + else => |e| return e, + }; + } + return File{ .handle = fd, .capable_io_mode = .blocking, @@ -887,6 +903,22 @@ pub const Dir = struct { }); } + if (has_flock_open_flags and flags.lock_nonblocking) { + var fl_flags = os.fcntl(fd, os.F_GETFL, 0) catch |err| switch (err) { + error.FileBusy => unreachable, + error.Locked => unreachable, + error.PermissionDenied => unreachable, + else => |e| return e, + }; + fl_flags &= ~@as(usize, os.O_NONBLOCK); + _ = os.fcntl(fd, os.F_SETFL, fl_flags) catch |err| switch (err) { + error.FileBusy => unreachable, + error.Locked => unreachable, + error.PermissionDenied => unreachable, + else => |e| return e, + }; + } + return File{ .handle = fd, .capable_io_mode = .blocking, From 26d20e39fcacda04b5f2e158dd38c293194e2f01 Mon Sep 17 00:00:00 2001 From: LemonBoy Date: Mon, 23 Nov 2020 09:11:03 +0100 Subject: [PATCH 3/4] std: Close dangling fd on error This patch was already submitted for openFileZ, createFileZ was left unpatched. --- lib/std/fs.zig | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/std/fs.zig b/lib/std/fs.zig index f927d53962..a20f9942f6 100644 --- a/lib/std/fs.zig +++ b/lib/std/fs.zig @@ -892,6 +892,7 @@ pub const Dir = struct { try std.event.Loop.instance.?.openatZ(self.fd, sub_path_c, os_flags, flags.mode) else try os.openatZ(self.fd, sub_path_c, os_flags, flags.mode); + errdefer os.close(fd); if (!has_flock_open_flags and flags.lock != .None) { // TODO: integrate async I/O From 8ed75614223ad86de26ddde9b9dda1ccbeb7d8d3 Mon Sep 17 00:00:00 2001 From: LemonBoy Date: Tue, 24 Nov 2020 10:23:54 +0100 Subject: [PATCH 4/4] std: Re-enable the use of O_EXLOCK/O_SHLOCK on macos --- lib/std/fs.zig | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/lib/std/fs.zig b/lib/std/fs.zig index a20f9942f6..64bd166f38 100644 --- a/lib/std/fs.zig +++ b/lib/std/fs.zig @@ -729,12 +729,14 @@ pub const Dir = struct { } var os_flags: u32 = os.O_CLOEXEC; - // Use the O_ locking flags if the os supports them - // (Or if it's darwin, as darwin's `open` doesn't support the O_SYNC flag) - const has_flock_open_flags = @hasDecl(os, "O_EXLOCK") and !is_darwin; + // Use the O_ locking flags if the os supports them to acquire the lock + // atomically. + const has_flock_open_flags = @hasDecl(os, "O_EXLOCK"); if (has_flock_open_flags) { + // Note that the O_NONBLOCK flag is removed after the openat() call + // is successful. const nonblocking_lock_flag: u32 = if (flags.lock_nonblocking) - os.O_NONBLOCK | os.O_SYNC + os.O_NONBLOCK else 0; os_flags |= switch (flags.lock) { @@ -870,11 +872,13 @@ pub const Dir = struct { return self.createFileW(path_w.span(), flags); } - // Use the O_ locking flags if the os supports them - // (Or if it's darwin, as darwin's `open` doesn't support the O_SYNC flag) - const has_flock_open_flags = @hasDecl(os, "O_EXLOCK") and !is_darwin; + // Use the O_ locking flags if the os supports them to acquire the lock + // atomically. + const has_flock_open_flags = @hasDecl(os, "O_EXLOCK"); + // Note that the O_NONBLOCK flag is removed after the openat() call + // is successful. const nonblocking_lock_flag: u32 = if (has_flock_open_flags and flags.lock_nonblocking) - os.O_NONBLOCK | os.O_SYNC + os.O_NONBLOCK else 0; const lock_flag: u32 = if (has_flock_open_flags) switch (flags.lock) {