Skip to content
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.

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

Implement readlink on Windows #5879

Merged
merged 28 commits into from
Jul 22, 2020
Merged

Implement readlink on Windows #5879

merged 28 commits into from
Jul 22, 2020

Conversation

kubkon
Copy link
Member

@kubkon kubkon commented Jul 15, 2020

This rather lengthy PR adds std.os.readlink support to Windows. At the same time, while developing basic smoke tests, this PR also reworks symlink on Windows, which now takes into account the updated Windows API. Finally, I've discovered after many failed attempts that on Windows, prepending \??\ is valid whenever we pass in the (absolute) path to NT-based syscalls such as NtCreateFile, however, this is no longer true when interacting with Win32 wrappers. It became apparent that when using std.os.windows.sliceToPrefixedFileW, the generated path passed to CreateSymbolicLinkW would be incorrectly treated as a relative path even thought it was absolute. Replacing the prefix from \??\ to \\?\ as described in MSDN manual fixed this behaviour. Therefore, I've added two new helper functions to std.os.windows module, namely sliceToWin32PrefixedFileW and cStrToWin32PrefixedFileW which behave exactly the same as sliceToPrefixedFileW and cStrToPrefixedFileW but prepend \\?\ instead. I guess it would be beneficial to rename sliceToPrefixedFileW into something like sliceToNtPrefixedFileW but IMHO this is job for a subsequent PR.

Aside from the above, this PR also tweaks and fixes:

  • tweaks std.os.unlinkatW to include FILE_OPEN_REPARSE_POINT flag without which deleting the actual symlink using NtCreateFile will erroneously delete the target or fail with error if the target is already gone,
  • adds third argument to std.os.symlink, namely, SymlinkFlags struct which is can be used to specify whether the symlink is meant to point at a file or directory. On Windows, it is an error if one tries to create a symlink to a directory without passing an appropriate flag to CreateSymbolicLinkW. This change is up for debate. In Rust, there are two Windows-only functions that achieve the same symlink_file and symlink_dir. Here I thought it'd be nice if we could instead provide a common interface across hosts, however, differentiate with flags which would be used only on selected hosts (here, this would only be Windows). However, there's still the remaning question of how do we handle disparity in error generation between hosts. POSIX has only one syscall which is symlink which can be used to create symlinks to both files and dirs, whereas on Windows, calling symlink without directory flags will likely cause an error. This one I still need to figure out but perhaps in a subsequent PR.
  • replaces FILE_TRANSACTED_MODE flag with FILE_OPEN_REPARSE_POINT flag in std.os.windows.bits. I couldn't find any mention of the former on MSDN, while at the same time, 0x00200000 is reserved for FILE_OPEN_REPARSE_POINT flag to be used in NtCreateFile syscall when opening the actual reparse point (symbolic link)

I'm not sure who to ask to review this, but if anyone who feels well-acquainted with Windows programming could have a look, that'd be awesome!

Copy link
Member

@andrewrk andrewrk left a comment

Choose a reason for hiding this comment

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

This looks like some really important work. I have a couple pieces of guidance here, I hope it makes sense.

lib/std/os.zig Outdated Show resolved Hide resolved
lib/std/os/windows.zig Outdated Show resolved Hide resolved
@kubkon
Copy link
Member Author

kubkon commented Jul 21, 2020

OK @andrewrk I think this is ready for the next round review. I've managed to re-implement CreateSymbolicLink and ReadLink using NT primitives, with the exception of DeviceIoControl. I plan to replace it with NtDeviceIoControl but I reckon this can be done in a subsequent PR. Does that sound fair?

Also, one thing I'm still unsure about is the behaviour of ReadLink when encountering an unhandled reparse point type. Currently, I've made it so that we return error.UnsupportedReparsePointType. However, I'm not sure if we should propagate it all the way to the client code such system.zig (and panic there), or do an outright panic in ReadLink itself.

Anyhow, have a look at the PR and let me know what you reckon!

@kubkon kubkon requested a review from andrewrk July 21, 2020 18:48
@daurnimator daurnimator added os-windows standard library This issue involves writing Zig code for the standard library. labels Jul 22, 2020
kubkon added 24 commits July 22, 2020 08:51
This way `std.fs.symlinkAbsolute` becomes cross-platform and we can
legally include `SymlinkFlags` as an argument that's only used on
Windows. Also, now `std.os.symlink` generates a compile error on
Windows with a message to instead use `std.os.windows.CreateSymbolicLink`.
Finally, this PR also reshuffles the tests between `std.os.test` and
`std.fs.test`.
Otherwise, the behaviour can lead to unexpected results, resulting
in removing an entire tree that's not necessarily under the root.
Furthermore, this change is needed if are to properly handle dir
symlinks on Windows. Without explicitly requiring that a directory
or file is opened with `FILE_OPEN_REPARSE_POINT`, Windows automatically
dereferences all symlinks along the way. This commit adds another
option to `OpenDirOptions`, namely `.no_follow`, which defaults to
`false` and can be used to specifically open a directory symlink on
Windows or call `openat` with `O_NOFOLLOW` flag in POSIX.
Fix WASI build, fix atomicSymlink by using `cwd().symLink`, add
`Dir.symLink` on supported targets.
@andrewrk andrewrk merged commit 9505bb7 into ziglang:master Jul 22, 2020
@andrewrk
Copy link
Member

@kubkon this is really, really nice work. This is a big deal for the std.fs API. Huge thanks for this PR.

@kubkon
Copy link
Member Author

kubkon commented Jul 22, 2020

@kubkon this is really, really nice work. This is a big deal for the std.fs API. Huge thanks for this PR.

My pleasure @andrewrk! :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
os-windows standard library This issue involves writing Zig code for the standard library.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants