From 0e3d74df8a8c06bcc0b1e4bff205f4ab8e4c6c13 Mon Sep 17 00:00:00 2001 From: Ryan Liptak Date: Fri, 26 Jun 2020 00:06:23 -0700 Subject: [PATCH 1/5] Add tests for using file operations on directories --- lib/std/fs/test.zig | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/lib/std/fs/test.zig b/lib/std/fs/test.zig index b024f0f4f6..87477549d9 100644 --- a/lib/std/fs/test.zig +++ b/lib/std/fs/test.zig @@ -73,6 +73,38 @@ test "directory operations on files" { file.close(); } +test "file operations on directories" { + var tmp_dir = tmpDir(.{}); + defer tmp_dir.cleanup(); + + const test_dir_name = "test_dir"; + + try tmp_dir.dir.makeDir(test_dir_name); + + testing.expectError(error.IsDir, tmp_dir.dir.createFile(test_dir_name, .{})); + testing.expectError(error.IsDir, tmp_dir.dir.deleteFile(test_dir_name)); + testing.expectError(error.IsDir, tmp_dir.dir.readFileAlloc(testing.allocator, test_dir_name, std.math.maxInt(usize))); + // note: the `.write = true` is necessary to ensure the error occurs on all platforms + testing.expectError(error.IsDir, tmp_dir.dir.openFile(test_dir_name, .{ .write = true })); + + if (builtin.os.tag != .wasi) { + // TODO: use Dir's realpath function once that exists + const absolute_path = blk: { + const relative_path = try fs.path.join(testing.allocator, &[_][]const u8{ "zig-cache", "tmp", tmp_dir.sub_path[0..], test_dir_name }); + defer testing.allocator.free(relative_path); + break :blk try fs.realpathAlloc(testing.allocator, relative_path); + }; + defer testing.allocator.free(absolute_path); + + testing.expectError(error.IsDir, fs.createFileAbsolute(absolute_path, .{})); + testing.expectError(error.IsDir, fs.deleteFileAbsolute(absolute_path)); + } + + // ensure the directory still exists as a sanity check + var dir = try tmp_dir.dir.openDir(test_dir_name, .{}); + dir.close(); +} + test "openSelfExe" { if (builtin.os.tag == .wasi) return error.SkipZigTest; From 14c3c47fb7315e6199751082f9ef544972bb13e6 Mon Sep 17 00:00:00 2001 From: Ryan Liptak Date: Fri, 26 Jun 2020 15:49:31 -0700 Subject: [PATCH 2/5] fs.deleteFile: Translate to error.IsDir when appropriate on POSIX systems Linux deviates from POSIX and returns EISDIR while other POSIX systems return EPERM. To make all platforms consistent in their errors when calling deleteFile on a directory, we have to do a stat to translate EPERM (AccessDenied) to EISDIR (IsDir). --- lib/std/fs.zig | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/lib/std/fs.zig b/lib/std/fs.zig index 6cb7d478b2..c574737194 100644 --- a/lib/std/fs.zig +++ b/lib/std/fs.zig @@ -1112,6 +1112,16 @@ pub const Dir = struct { pub fn deleteFile(self: Dir, sub_path: []const u8) DeleteFileError!void { os.unlinkat(self.fd, sub_path, 0) catch |err| switch (err) { error.DirNotEmpty => unreachable, // not passing AT_REMOVEDIR + error.AccessDenied => |e| switch (builtin.os.tag) { + // non-Linux POSIX systems return EPERM when trying to delete a directory, so + // we need to handle that case specifically and translate the error + .macosx, .ios, .freebsd, .netbsd, .dragonfly => { + const fstat = os.fstatat(self.fd, sub_path, 0) catch return e; + const is_dir = fstat.mode & os.S_IFMT == os.S_IFDIR; + return if (is_dir) error.IsDir else e; + }, + else => return e, + }, else => |e| return e, }; } @@ -1122,6 +1132,16 @@ pub const Dir = struct { pub fn deleteFileZ(self: Dir, sub_path_c: [*:0]const u8) DeleteFileError!void { os.unlinkatZ(self.fd, sub_path_c, 0) catch |err| switch (err) { error.DirNotEmpty => unreachable, // not passing AT_REMOVEDIR + error.AccessDenied => |e| switch (builtin.os.tag) { + // non-Linux POSIX systems return EPERM when trying to delete a directory, so + // we need to handle that case specifically and translate the error + .macosx, .ios, .freebsd, .netbsd, .dragonfly => { + const fstat = os.fstatatZ(self.fd, sub_path_c, 0) catch return e; + const is_dir = fstat.mode & os.S_IFMT == os.S_IFDIR; + return if (is_dir) error.IsDir else e; + }, + else => return e, + }, else => |e| return e, }; } From 505bc9817a2b78c97819f481b3cd98102c7ff964 Mon Sep 17 00:00:00 2001 From: Ryan Liptak Date: Fri, 26 Jun 2020 16:08:26 -0700 Subject: [PATCH 3/5] Implement Dir.deleteFile in terms of deleteFileZ/deleteFileW Reduces duplicate code, consistent with other fn/fnZ/fnW implementations --- lib/std/fs.zig | 26 ++++++++++++-------------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/lib/std/fs.zig b/lib/std/fs.zig index c574737194..e7de8bd52d 100644 --- a/lib/std/fs.zig +++ b/lib/std/fs.zig @@ -1110,20 +1110,18 @@ pub const Dir = struct { /// Delete a file name and possibly the file it refers to, based on an open directory handle. /// Asserts that the path parameter has no null bytes. pub fn deleteFile(self: Dir, sub_path: []const u8) DeleteFileError!void { - os.unlinkat(self.fd, sub_path, 0) catch |err| switch (err) { - error.DirNotEmpty => unreachable, // not passing AT_REMOVEDIR - error.AccessDenied => |e| switch (builtin.os.tag) { - // non-Linux POSIX systems return EPERM when trying to delete a directory, so - // we need to handle that case specifically and translate the error - .macosx, .ios, .freebsd, .netbsd, .dragonfly => { - const fstat = os.fstatat(self.fd, sub_path, 0) catch return e; - const is_dir = fstat.mode & os.S_IFMT == os.S_IFDIR; - return if (is_dir) error.IsDir else e; - }, - else => return e, - }, - else => |e| return e, - }; + if (builtin.os.tag == .windows) { + const sub_path_w = try os.windows.sliceToPrefixedFileW(sub_path); + return self.deleteFileW(sub_path_w.span().ptr); + } else if (builtin.os.tag == .wasi) { + os.unlinkatWasi(self.fd, sub_path, 0) catch |err| switch (err) { + error.DirNotEmpty => unreachable, // not passing AT_REMOVEDIR + else => |e| return e, + }; + } else { + const sub_path_c = try os.toPosixPath(sub_path); + return self.deleteFileZ(&sub_path_c); + } } pub const deleteFileC = @compileError("deprecated: renamed to deleteFileZ"); From 12aca758c6ee923065d1d64a62e44c8a8fd6cd99 Mon Sep 17 00:00:00 2001 From: Ryan Liptak Date: Fri, 26 Jun 2020 17:12:02 -0700 Subject: [PATCH 4/5] Dir.deleteFile: Fix symlink behavior when translating EPERM to EISDIR --- lib/std/fs.zig | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/std/fs.zig b/lib/std/fs.zig index e7de8bd52d..28f2daa96a 100644 --- a/lib/std/fs.zig +++ b/lib/std/fs.zig @@ -1134,7 +1134,8 @@ pub const Dir = struct { // non-Linux POSIX systems return EPERM when trying to delete a directory, so // we need to handle that case specifically and translate the error .macosx, .ios, .freebsd, .netbsd, .dragonfly => { - const fstat = os.fstatatZ(self.fd, sub_path_c, 0) catch return e; + // Don't follow symlinks to match unlinkat (which acts on symlinks rather than follows them) + const fstat = os.fstatatZ(self.fd, sub_path_c, os.AT_SYMLINK_NOFOLLOW) catch return e; const is_dir = fstat.mode & os.S_IFMT == os.S_IFDIR; return if (is_dir) error.IsDir else e; }, From 74c245aea94ebcf05651a6f3420d8c3a7145f9d7 Mon Sep 17 00:00:00 2001 From: Ryan Liptak Date: Sun, 28 Jun 2020 13:56:00 -0700 Subject: [PATCH 5/5] Disable wasi 'readFileAlloc on a directory' assertion for now --- lib/std/fs/test.zig | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/lib/std/fs/test.zig b/lib/std/fs/test.zig index 87477549d9..3991ddd8a3 100644 --- a/lib/std/fs/test.zig +++ b/lib/std/fs/test.zig @@ -83,8 +83,13 @@ test "file operations on directories" { testing.expectError(error.IsDir, tmp_dir.dir.createFile(test_dir_name, .{})); testing.expectError(error.IsDir, tmp_dir.dir.deleteFile(test_dir_name)); - testing.expectError(error.IsDir, tmp_dir.dir.readFileAlloc(testing.allocator, test_dir_name, std.math.maxInt(usize))); - // note: the `.write = true` is necessary to ensure the error occurs on all platforms + // Currently, WASI will return error.Unexpected (via ENOTCAPABLE) when attempting fd_read on a directory handle. + // TODO: Re-enable on WASI once https://github.com/bytecodealliance/wasmtime/issues/1935 is resolved. + if (builtin.os.tag != .wasi) { + testing.expectError(error.IsDir, tmp_dir.dir.readFileAlloc(testing.allocator, test_dir_name, std.math.maxInt(usize))); + } + // Note: The `.write = true` is necessary to ensure the error occurs on all platforms. + // TODO: Add a read-only test as well, see https://github.com/ziglang/zig/issues/5732 testing.expectError(error.IsDir, tmp_dir.dir.openFile(test_dir_name, .{ .write = true })); if (builtin.os.tag != .wasi) {