Skip to content

Commit

Permalink
std.fs: Get some more rename tests passing on Windows
Browse files Browse the repository at this point in the history
Most of these tests were made to pass by ziglang#16717, but there were a few loose ends to tie up with regards to the 'fallback' behavior (when FILE_RENAME_POSIX_SEMANTICS either isn't available or isn't supported by the underlying filesystem).

We now do a bit of extra work to get POSIX-like renaming in the fallback path.

Closes ziglang#6364
  • Loading branch information
squeek502 committed Jan 21, 2024
1 parent 5c4cb60 commit e27ccd6
Show file tree
Hide file tree
Showing 3 changed files with 68 additions and 11 deletions.
5 changes: 4 additions & 1 deletion lib/std/fs/Dir.zig
Original file line number Diff line number Diff line change
Expand Up @@ -1626,7 +1626,10 @@ pub const RenameError = posix.RenameError;
/// Change the name or location of a file or directory.
/// If new_sub_path already exists, it will be replaced.
/// Renaming a file over an existing directory or a directory
/// over an existing file will fail with `error.IsDir` or `error.NotDir`
/// over an existing file will fail with `error.IsDir` or `error.NotDir`.
/// Renaming a directory over an existing directory will succeed if
/// new_sub_path is an empty directory, or fail with `error.PathAlreadyExists`
/// if new_sub_path not an empty directory.
pub fn rename(self: Dir, old_sub_path: []const u8, new_sub_path: []const u8) RenameError!void {
return posix.renameat(self.fd, old_sub_path, self.fd, new_sub_path);
}
Expand Down
9 changes: 0 additions & 9 deletions lib/std/fs/test.zig
Original file line number Diff line number Diff line change
Expand Up @@ -852,9 +852,6 @@ test "Dir.rename directories" {
}

test "Dir.rename directory onto empty dir" {
// TODO: Fix on Windows, see https://github.com/ziglang/zig/issues/6364
if (builtin.os.tag == .windows) return error.SkipZigTest;

try testWithAllSupportedPathTypes(struct {
fn impl(ctx: *TestContext) !void {
const test_dir_path = try ctx.transformPath("test_dir");
Expand All @@ -873,9 +870,6 @@ test "Dir.rename directory onto empty dir" {
}

test "Dir.rename directory onto non-empty dir" {
// TODO: Fix on Windows, see https://github.com/ziglang/zig/issues/6364
if (builtin.os.tag == .windows) return error.SkipZigTest;

try testWithAllSupportedPathTypes(struct {
fn impl(ctx: *TestContext) !void {
const test_dir_path = try ctx.transformPath("test_dir");
Expand All @@ -899,9 +893,6 @@ test "Dir.rename directory onto non-empty dir" {
}

test "Dir.rename file <-> dir" {
// TODO: Fix on Windows, see https://github.com/ziglang/zig/issues/6364
if (builtin.os.tag == .windows) return error.SkipZigTest;

try testWithAllSupportedPathTypes(struct {
fn impl(ctx: *TestContext) !void {
const test_file_path = try ctx.transformPath("test_file");
Expand Down
65 changes: 64 additions & 1 deletion lib/std/os.zig
Original file line number Diff line number Diff line change
Expand Up @@ -2795,7 +2795,7 @@ pub fn renameatW(
) RenameError!void {
const src_fd = windows.OpenFile(old_path_w, .{
.dir = old_dir_fd,
.access_mask = windows.SYNCHRONIZE | windows.GENERIC_WRITE | windows.DELETE,
.access_mask = windows.SYNCHRONIZE | windows.GENERIC_WRITE | windows.DELETE | windows.GENERIC_READ,
.creation = windows.FILE_OPEN,
.io_mode = .blocking,
.filter = .any, // This function is supposed to rename both files and directories.
Expand Down Expand Up @@ -2852,6 +2852,69 @@ pub fn renameatW(
}

if (need_fallback) {
// FileRenameInformation has surprising behavior around renaming a directory to the
// path of a file: it succeeds and replaces the file with the directory.
//
// To avoid this behavior and instead return error.NotDir, we (unfortunately) have
// to query the type of both the source and destination. However, doing so also
// allows us to return error.IsDir/error.PathAlreadyExists in cases where NtSetInformationFile
// with FileRenameInformation would have only returned a generic ACCESS_DENIED.
const src_kind = src_kind: {
const src_file = fs.File{ .handle = src_fd, .capable_io_mode = .blocking, .intended_io_mode = .blocking };
break :src_kind (try src_file.stat()).kind;
};

const maybe_dest_kind: ?fs.File.Kind = dest_kind: {
const dest_fd = windows.OpenFile(new_path_w, .{
.dir = new_dir_fd,
.access_mask = windows.SYNCHRONIZE | windows.GENERIC_READ | windows.DELETE,
.creation = windows.FILE_OPEN,
.io_mode = .blocking,
.filter = .any, // This function is supposed to rename both files and directories.
.follow_symlinks = false,
}) catch |err| switch (err) {
error.FileNotFound => break :dest_kind null,
error.WouldBlock => unreachable, // Not possible without `.share_access_nonblocking = true`.
else => |e| return e,
};
defer windows.CloseHandle(dest_fd);

const dest_file = fs.File{ .handle = dest_fd, .capable_io_mode = .blocking, .intended_io_mode = .blocking };
const dest_kind = (try dest_file.stat()).kind;

// To match POSIX behavior, we want to be able to rename directories onto empty directories,
// so if the src and dest are both directories, we try deleting the dest (which will
// fail if the dest dir is non-empty). Hwoever, any error when attempting to delete
// can be treated the same, since any failure to delete here will lead to the rename
// failing anyway.
if (src_kind == .directory and dest_kind == .directory) {
var file_dispo = windows.FILE_DISPOSITION_INFORMATION{
.DeleteFile = windows.TRUE,
};

var io: windows.IO_STATUS_BLOCK = undefined;
rc = windows.ntdll.NtSetInformationFile(
dest_fd,
&io,
&file_dispo,
@sizeOf(windows.FILE_DISPOSITION_INFORMATION),
.FileDispositionInformation,
);

switch (rc) {
.SUCCESS => break :dest_kind null,
else => return error.PathAlreadyExists,
}
}

break :dest_kind dest_kind;
};

if (maybe_dest_kind) |dest_kind| {
if (src_kind == .file and dest_kind == .directory) return error.IsDir;
if (src_kind == .directory and dest_kind == .file) return error.NotDir;
}

const struct_buf_len = @sizeOf(windows.FILE_RENAME_INFORMATION) + (MAX_PATH_BYTES - 1);
var rename_info_buf: [struct_buf_len]u8 align(@alignOf(windows.FILE_RENAME_INFORMATION)) = undefined;
const struct_len = @sizeOf(windows.FILE_RENAME_INFORMATION) - 1 + new_path_w.len * 2;
Expand Down

0 comments on commit e27ccd6

Please sign in to comment.