Skip to content

Remove all fs.<foo>Absolute usages throughout the Zig codebase #16743

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.

Sign up for GitHub

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

Closed
squeek502 wants to merge 1 commit into from
Closed

Remove all fs.<foo>Absolute usages throughout the Zig codebase #16743

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 3 additions & 25 deletions lib/std/crypto/Certificate/Bundle.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ fn rescanLinux(cb: *Bundle, gpa: Allocator) RescanLinuxError!void {

scan: {
for (cert_file_paths) |cert_file_path| {
if (addCertsFromFilePathAbsolute(cb, gpa, cert_file_path)) |_| {
if (addCertsFromFilePath(cb, gpa, std.fs.cwd(), cert_file_path)) |_| {
break :scan;
} else |err| switch (err) {
error.FileNotFound => continue,
Expand All @@ -105,7 +105,7 @@ fn rescanLinux(cb: *Bundle, gpa: Allocator) RescanLinuxError!void {
}

for (cert_dir_paths) |cert_dir_path| {
addCertsFromDirPathAbsolute(cb, gpa, cert_dir_path) catch |err| switch (err) {
addCertsFromDirPath(cb, gpa, std.fs.cwd(), cert_dir_path) catch |err| switch (err) {
error.FileNotFound => continue,
else => |e| return e,
};
Expand All @@ -120,7 +120,7 @@ const RescanBSDError = AddCertsFromFilePathError;
fn rescanBSD(cb: *Bundle, gpa: Allocator, cert_file_path: []const u8) RescanBSDError!void {
cb.bytes.clearRetainingCapacity();
cb.map.clearRetainingCapacity();
try addCertsFromFilePathAbsolute(cb, gpa, cert_file_path);
try addCertsFromFilePath(cb, gpa, std.fs.cwd(), cert_file_path);
cb.bytes.shrinkAndFree(gpa, cb.bytes.items.len);
}

Expand Down Expand Up @@ -164,17 +164,6 @@ pub fn addCertsFromDirPath(
return addCertsFromDir(cb, gpa, iterable_dir);
}

pub fn addCertsFromDirPathAbsolute(
cb: *Bundle,
gpa: Allocator,
abs_dir_path: []const u8,
) AddCertsFromDirPathError!void {
assert(fs.path.isAbsolute(abs_dir_path));
var iterable_dir = try fs.openIterableDirAbsolute(abs_dir_path, .{});
defer iterable_dir.close();
return addCertsFromDir(cb, gpa, iterable_dir);
}

pub const AddCertsFromDirError = AddCertsFromFilePathError;

pub fn addCertsFromDir(cb: *Bundle, gpa: Allocator, iterable_dir: fs.IterableDir) AddCertsFromDirError!void {
Expand All @@ -191,17 +180,6 @@ pub fn addCertsFromDir(cb: *Bundle, gpa: Allocator, iterable_dir: fs.IterableDir

pub const AddCertsFromFilePathError = fs.File.OpenError || AddCertsFromFileError;

pub fn addCertsFromFilePathAbsolute(
cb: *Bundle,
gpa: Allocator,
abs_file_path: []const u8,
) AddCertsFromFilePathError!void {
assert(fs.path.isAbsolute(abs_file_path));
var file = try fs.openFileAbsolute(abs_file_path, .{});
defer file.close();
return addCertsFromFile(cb, gpa, file);
}

pub fn addCertsFromFilePath(
cb: *Bundle,
gpa: Allocator,
Expand Down
2 changes: 1 addition & 1 deletion lib/std/crypto/Certificate/Bundle/macos.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ pub fn rescanMac(cb: *Bundle, gpa: Allocator) RescanMacError!void {
cb.bytes.clearRetainingCapacity();
cb.map.clearRetainingCapacity();

const file = try fs.openFileAbsolute("/System/Library/Keychains/SystemRootCertificates.keychain", .{});
const file = try fs.cwd().openFile("/System/Library/Keychains/SystemRootCertificates.keychain", .{});
defer file.close();

const bytes = try file.readToEndAlloc(gpa, std.math.maxInt(u32));
Expand Down
11 changes: 3 additions & 8 deletions lib/std/debug.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1071,13 +1071,8 @@ pub fn readElfDebugInfo(
parent_mapped_mem: ?[]align(mem.page_size) const u8,
) !ModuleDebugInfo {
nosuspend {

// TODO https://github.com/ziglang/zig/issues/5525
Copy link
Member Author

@squeek502 squeek502 Aug 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This TODO was added in fd067fb. The current code no longer used that workaround, so this TODO didn't seem relevant anymore.

const elf_file = (if (elf_filename) |filename| blk: {
break :blk if (fs.path.isAbsolute(filename))
fs.openFileAbsolute(filename, .{ .intended_io_mode = .blocking })
else
fs.cwd().openFile(filename, .{ .intended_io_mode = .blocking });
break :blk fs.cwd().openFile(filename, .{ .intended_io_mode = .blocking });
} else fs.openSelfExe(.{ .intended_io_mode = .blocking })) catch |err| switch (err) {
error.FileNotFound => return error.MissingDebugInfo,
else => return err,
Expand Down Expand Up @@ -1702,7 +1697,7 @@ pub const DebugInfo = struct {
// a binary is produced with -gdwarf, since the section names are longer than 8 bytes.
if (coff_obj.strtabRequired()) {
var name_buffer: [windows.PATH_MAX_WIDE + 4:0]u16 = undefined;
// openFileAbsoluteW requires the prefix to be present
// openFileW requires the NT namespace prefix to be present
mem.copy(u16, name_buffer[0..4], &[_]u16{ '\\', '?', '?', '\\' });

const process_handle = windows.kernel32.GetCurrentProcess();
Expand All @@ -1714,7 +1709,7 @@ pub const DebugInfo = struct {
);

if (len == 0) return error.MissingDebugInfo;
const coff_file = fs.openFileAbsoluteW(name_buffer[0 .. len + 4 :0], .{}) catch |err| switch (err) {
const coff_file = fs.cwd().openFileW(name_buffer[0 .. len + 4 :0], .{}) catch |err| switch (err) {
error.FileNotFound => return error.MissingDebugInfo,
else => return err,
};
Expand Down
4 changes: 2 additions & 2 deletions lib/std/net.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1242,7 +1242,7 @@ fn linuxLookupNameFromHosts(
family: os.sa_family_t,
port: u16,
) !void {
const file = fs.openFileAbsoluteZ("/etc/hosts", .{}) catch |err| switch (err) {
const file = fs.cwd().openFileZ("/etc/hosts", .{}) catch |err| switch (err) {
error.FileNotFound,
error.NotDir,
error.AccessDenied,
Expand Down Expand Up @@ -1443,7 +1443,7 @@ fn getResolvConf(allocator: mem.Allocator, rc: *ResolvConf) !void {
};
errdefer rc.deinit();

const file = fs.openFileAbsoluteZ("/etc/resolv.conf", .{}) catch |err| switch (err) {
const file = fs.cwd().openFileZ("/etc/resolv.conf", .{}) catch |err| switch (err) {
error.FileNotFound,
error.NotDir,
error.AccessDenied,
Expand Down
4 changes: 2 additions & 2 deletions lib/std/process.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -967,7 +967,7 @@ pub fn getUserInfo(name: []const u8) !UserInfo {
/// TODO this reads /etc/passwd. But sometimes the user/id mapping is in something else
/// like NIS, AD, etc. See `man nss` or look at an strace for `id myuser`.
pub fn posixGetUserInfo(name: []const u8) !UserInfo {
const file = try std.fs.openFileAbsolute("/etc/passwd", .{});
const file = try std.fs.cwd().openFile("/etc/passwd", .{});
Copy link
Member

@andrewrk andrewrk Sep 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a good example of something that's not an improvement.

The code before will use the open() syscall while the code after will use the openat() syscall with AT_FDCWD. In practice this is generally fine, however, the latter introduces a dependency on the concept of the current working directory. The current working directory, however, is irrelevant to reading this absolute file path.

Another way to put this is that the code before specified the intent more precisely, while the code after specifies intent less precisely.

It is for this reason that I think the "absolute" variants of fs functions should continue to exist.

Copy link
Member Author

@squeek502 squeek502 Sep 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code before will use the open() syscall

This is not true in this case:

zig/lib/std/fs.zig

Lines 2761 to 2764 in f40f81c

pub fn openFileAbsolute(absolute_path: []const u8, flags: File.OpenFlags) File.OpenError!File {
assert(path.isAbsolute(absolute_path));
return cwd().openFile(absolute_path, flags);
}

The only Absolute functions that are not purely wrappers around the Dir function are:

  • readLinkAbsolute, symLinkAbsolute, makeDirAbsolute, deleteDirAbsolute, renameAbsolute

All the rest look just like openFileAbsolute. For all the changes in this PR, the strace would actually be identical before and after.

Another way to put this is that the code before specified the intent more precisely, while the code after specifies intent less precisely.

I came to the opposite conclusion when writing this PR. From #16736 (comment):

In making the changes in [this] PR, I came across another reason why the Absolute versions aren't very useful: the Absolute terminology is overloaded and it's hard/impossible to tell why the Absolute variant is being used--is it being used just because the programmer happens to know the path is absolute, or is it being used because the programmer knows the path must be absolute for the code to work correctly? Removing the Absolute variants will remove this ambiguity since the programmer will be forced to make their intentions more explicit if they want to assert/check that a given path is absolute.

Copy link
Contributor

@daurnimator daurnimator Sep 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll also add that on some architectures, non -at versions of many syscalls don't exist.

Copy link
Member

@andrewrk andrewrk Nov 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the rest look just like openFileAbsolute. For all the changes in this PR, the strace would actually be identical before and after.

It would be an improvement if the implementation were changed to match what I said, but it doesn't matter too much if the OS treats it the same.

More importantly, it is an impedance mismatch to call fs.cwd() in order to do something with an absolute file path. Looking at the implementation of cwd:

pub fn cwd() Dir {
    if (builtin.os.tag == .windows) {
        return Dir{ .fd = os.windows.peb().ProcessParameters.CurrentDirectory.Handle };
    } else if (builtin.os.tag == .wasi) {
        return std.options.wasiCwd();
    } else {
        return Dir{ .fd = os.AT.FDCWD };
    }
}

We can observe some problems. On Windows, this lowers ultimately to some inline assembly (that is apparently incorrectly marked as having side effects, see #18077 that I just opened) that should not be emitted when accessing an absolute path. On WASI it could cause all sorts of unintended problems because it hooks into a user-overridable function for getting the current working directory.

These things are minor issues, but they are the consequences of specifying the wrong intent.

I don't really understand your second point. The functions dealing with absolute paths assert that the parameter is an absolute path, so I think you have it precisely backwards - using the absolute functions is the way to make intentions more explicit.

Copy link
Member Author

@squeek502 squeek502 Nov 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I understand where you're coming from better now. I was mostly focused on the potential downstream benefits of there being one obvious way of doing fs-related stuff, but I can understand why avoiding a known-to-be-unnecessary fs.cwd() call can be beneficial.

Before I close this, what are your thoughts on this pattern:

zig/lib/std/debug.zig

Lines 1077 to 1080 in ac95cfe

break :blk if (fs.path.isAbsolute(filename))
fs.openFileAbsolute(filename, .{ .intended_io_mode = .blocking })
else
fs.cwd().openFile(filename, .{ .intended_io_mode = .blocking });

Do you see that as something that should be done more, or should that be replaced with fs.cwd().openFile?

Copy link
Member

@andrewrk andrewrk Nov 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that pattern should always be replaced with fs.cwd().openFile.

defer file.close();

const reader = file.reader();
Expand Down Expand Up @@ -1207,7 +1207,7 @@ pub fn totalSystemMemory() TotalSystemMemoryError!usize {
}

fn totalSystemMemoryLinux() !usize {
var file = try std.fs.openFileAbsoluteZ("/proc/meminfo", .{});
var file = try std.fs.cwd().openFileZ("/proc/meminfo", .{});
defer file.close();
var buf: [50]u8 = undefined;
const amt = try file.read(&buf);
Expand Down
2 changes: 1 addition & 1 deletion lib/std/zig/system/NativeTargetInfo.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -320,7 +320,7 @@ fn detectAbiAndDynamicLinker(
// #! (2) + 255 (max length of shebang line since Linux 5.1) + \n (1)
var buffer: [258]u8 = undefined;
while (true) {
const file = fs.openFileAbsolute(file_name, .{}) catch |err| switch (err) {
const file = fs.cwd().openFile(file_name, .{}) catch |err| switch (err) {
error.NoSpaceLeft => unreachable,
error.NameTooLong => unreachable,
error.PathAlreadyExists => unreachable,
Expand Down
2 changes: 1 addition & 1 deletion lib/std/zig/system/linux.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -331,7 +331,7 @@ fn CpuinfoParser(comptime impl: anytype) type {
}

pub fn detectNativeCpuAndFeatures() ?Target.Cpu {
var f = fs.openFileAbsolute("/proc/cpuinfo", .{ .intended_io_mode = .blocking }) catch |err| switch (err) {
var f = fs.cwd().openFile("/proc/cpuinfo", .{ .intended_io_mode = .blocking }) catch |err| switch (err) {
else => return null,
};
defer f.close();
Expand Down
6 changes: 3 additions & 3 deletions src/windows_sdk.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -476,7 +476,7 @@ pub const Windows81Sdk = struct {
if (!std.fs.path.isAbsolute(sdk_lib_dir_path)) return error.Windows81SdkNotFound;

// enumerate files in sdk path looking for latest version
var sdk_lib_dir = std.fs.openIterableDirAbsolute(sdk_lib_dir_path, .{}) catch |err| switch (err) {
var sdk_lib_dir = std.fs.cwd().openIterableDir(sdk_lib_dir_path, .{}) catch |err| switch (err) {
error.NameTooLong => return error.PathTooLong,
else => return error.Windows81SdkNotFound,
};
Expand Down Expand Up @@ -727,7 +727,7 @@ const MsvcLibDir = struct {
if (!std.fs.path.isAbsolute(visualstudio_folder_path)) return error.PathNotFound;
// enumerate folders that contain `privateregistry.bin`, looking for all versions
// f.i. %localappdata%\Microsoft\VisualStudio\17.0_9e9cbb98\
var visualstudio_folder = std.fs.openIterableDirAbsolute(visualstudio_folder_path, .{}) catch return error.PathNotFound;
var visualstudio_folder = std.fs.cwd().openIterableDir(visualstudio_folder_path, .{}) catch return error.PathNotFound;
defer visualstudio_folder.close();

var iterator = visualstudio_folder.iterate();
Expand Down Expand Up @@ -874,7 +874,7 @@ const MsvcLibDir = struct {
fn verifyLibDir(lib_dir_path: []const u8) bool {
std.debug.assert(std.fs.path.isAbsolute(lib_dir_path)); // should be already handled in `findVia*`

var dir = std.fs.openDirAbsolute(lib_dir_path, .{}) catch return false;
var dir = std.fs.cwd().openDir(lib_dir_path, .{}) catch return false;
defer dir.close();

const stat = dir.statFile("vcruntime.lib") catch return false;
Expand Down
2 changes: 1 addition & 1 deletion tools/generate_linux_syscalls.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ pub fn main() !void {
// As of 5.17.1, the largest table is 23467 bytes.
// 32k should be enough for now.
var buf = try allocator.alloc(u8, 1 << 15);
const linux_dir = try std.fs.openDirAbsolute(linux_path, .{});
const linux_dir = try std.fs.cwd().openDir(linux_path, .{});

try writer.writeAll(
\\// This file is automatically generated.
Expand Down