Add std.os.getFdPath and std.fs.Dir.realpath by kubkon · Pull Request #5701 · ziglang/zig

kubkon · 2020-06-24T18:58:07Z

~~std.os.realpathat is similar to std.os.realpath, however, it accepts~~
~~a pair fd_t, []const u8 of args and thus works out the realpath~~
~~of a relative path wrt to some opened file descriptor.~~

~~If the input pathname argument turns out to be an absolute path, this~~
~~function reverts to calling realpath on that pathname completely~~
~~ignoring the input file descriptor. This behaviour is standard in~~
~~POSIX and IMHO a good rule of thumb to follow.~~

~~If the input file descriptor was obtained using std.fs.cwd() call,~~
~~this function reverts to std.os.getcwd() to obtain the file~~
~~descriptor's path.~~

~~std.fs.Dir.realpath integrates std.os.realpathat with std.fs.Dir~~
~~but with dynamic memory allocator.~~

~~Part 1 of #5636.~~

EDIT:

See the comments for an update of the contents of this PR.

andrewrk · 2020-06-25T02:29:07Z

The Windows failure here looks legit

kubkon · 2020-06-25T07:16:23Z

The Windows failure here looks legit

Thanks for having a look! Windows still surprises me at times. Anyhow, I've fixed the tests. I was wondering though, should this be documented somewhere, or is this common knowledge (or at least expected) on Windows (that running std.os.realpath on path that we hold a handle to yields a sharing violation)?

kubkon · 2020-06-26T04:53:14Z

@squeek502, since #5684 landed, I've gone ahead and fixed the TODO. :-)

squeek502 · 2020-06-27T06:13:04Z

lib/std/fs.zig

+    /// relative to this `Dir`. If `pathname` is absolute, ignores this `Dir` handle
+    /// and returns the canonicalized absolute pathname of `pathname` argument.
+    /// Caller must free the returned memory.
+    pub fn realpath(self: Dir, allocator: *Allocator, pathname: []const u8) ![]u8 {


Following conventions, I think this should be realpathAlloc and there should be a realpath, realpathZ, and realpathW that take a slice as a buffer. See std.fs.realpath and friends:

zig/lib/std/fs.zig

Lines 26 to 29 in bb55889

pub const realpath = os.realpath;

pub const realpathZ = os.realpathZ;

pub const realpathC = @compileError("deprecated: renamed to realpathZ");

pub const realpathW = os.realpathW;

Excellent point, thanks! Will tweak and report back.

@squeek502 I've now added first pass at implementing your suggestion. It'd be great if you could have a look and lemme know if there's any missing, or if something could be improved upon, etc.

I didn't realize that the os.realpath functions use *[MAX_PATH_BYTES]u8 for their out_buffer params. Changing Dir.realpath and friends to use that instead of []u8 would allow for removing the intermediate buffers in their implementations which seems nicer to me. Definitely worth waiting for @andrewrk to chime in on that before changing it though.

Oh, yeah, that’s correct. It’d be great to change the interface from array to slice in std.os.realpath and friends but my suggestion would be doing it in a subsequent PR. Would that make sense?

lib/std/fs.zig

kubkon · 2020-07-04T09:53:12Z

Just a heads up that #5784 provides a fix for some incorrect handling of relative components such as . and .. in pathname argument to Windows-only os.realpathatW. #5784 also provides tests to that effect.

andrewrk

Nice work @kubkon.

I want to be careful about how to do this. It looks like realpathat is not a widely portable abstraction that we can provide. Now, that does not mean that we cannot provide it, but it should be very clear, at least from the doc comments, that this is not universally supported API, and will cause compile errors on some operating systems.

Furthermore - this API should be avoided for the cache hash API, because for our purposes there, we do want a widely portable API that works on all OS's.

Does this make sense? I can take a look at the cache hash PR and provide some guidance there too, if that would help.

andrewrk · 2020-07-08T19:52:23Z

lib/std/os.zig

+        else => {
+            var procfs_buf: ["/proc/self/fd/-2147483648".len:0]u8 = undefined;
+            const proc_path = std.fmt.bufPrint(procfs_buf[0..], "/proc/self/fd/{}\x00", .{fd}) catch unreachable;
+
+            return readlinkZ(@ptrCast([*:0]const u8, proc_path.ptr), out_buffer);
+        },


This is Linux-specific logic so it should go into a .linux => prong. The one that goes into else should be one that looks like it would work for POSIX systems in general, which, in this case, would be the macos prong. However I looked into it a bit and it seems that other operating systems *do not support the concept of realpathat, which makes me want to be very careful about how the std lib goes about this.

This should be a compile error on operating systems that don't support this API

That's an excellent point, thanks for pointing it out @andrewrk. I'm not sure why I erroneously assumed that for instance, what is true for Linux is also true for BSDs (looking at you here, FreeBSD). All in all, I agree with you here 100%, and I'll make sure to have any other host return a compile error on an attempt at using realpathat. For more context and for anyone joining in in this discussion, FreeBSD is an example of an OS which generally doesn't (officially at least, and in a non-hacky way) support querying the OS for a canonical path of a file descriptor. One excellent source describing this, can be found here. BTW, I'll make sure to add this to the docs.

@andrewrk OK, I've made sure we throw a compile error for all unsupported hosts. In other words, we currently support linux, macos and windows, and for all others, it's a compile error.

While we're here, do you think we should explicitly return a compile error in std.os.realpathat for unsupported hosts as well, or let it drop down to fdPath and the error bubble up from them?

I've renamed fdPath to getFdPath and I've tweaked the docs to clearly state that both getFdPath and realpathat are not supported on all hosts.

kubkon · 2020-07-08T20:08:42Z

Nice work @kubkon.

I want to be careful about how to do this. It looks like realpathat is not a widely portable abstraction that we can provide. Now, that does not mean that we cannot provide it, but it should be very clear, at least from the doc comments, that this is not universally supported API, and will cause compile errors on some operating systems.

Furthermore - this API should be avoided for the cache hash API, because for our purposes there, we do want a widely portable API that works on all OS's.

Does this make sense? I can take a look at the cache hash PR and provide some guidance there too, if that would help.

Makes sense! So #5636 does provide a couple of bug fixes to realpathat plus provides a WASI-specific implementation, but I'd be happy to merge the changes that deal with realpathat (plus any additional tests) into this PR. Or I could revert changes to the CacheHash struct, and we could move the conversation from here to #5636. Lemme know what works best in your opinion!

andrewrk · 2020-07-08T20:10:09Z

Lemme know what works best in your opinion!

hmm, if you don't mind, let's focus on this API layer first, and make sure we feel good about it, then move on to cache hash

kubkon · 2020-07-08T20:12:53Z

Lemme know what works best in your opinion!

hmm, if you don't mind, let's focus on this API layer first, and make sure we feel good about it, then move on to cache hash

@andrewrk Right, that's what I meant, sorry. Lemme clarify a bit more. So when working on cache hash, I've also added a couple of niceties to realpathat and those landed in the other PR. And what I meant was, I could probably move those changes from the other PR to this one since they deal with realpathat only.

andrewrk · 2020-07-08T20:14:32Z

I could probably move those changes from the other PR to this one since they deal with realpathat only.

Yeah that sounds good 👍

kubkon · 2020-07-08T20:15:31Z

I could probably move those changes from the other PR to this one since they deal with realpathat only.

Yeah that sounds good 👍

Cool, I'll get on it then! Thanks for your patience, and sorry about the confusion! :-)

kubkon · 2020-07-08T21:08:14Z

@andrewrk commits migrated! So now, realpathat also work in WASI, although with this twist that the realpath is resolved upto the Dir handle (anything beyond is considered an attempt at escaping the sandbox, and hence erroneous). I've made sure this is documented appropriately. While this may sound like a weird thing, I noticed that realpathat is the closest thing to "canonicalize" in other languages, and we should definitely provide something like this for WASI.

kubkon · 2020-07-09T06:24:37Z

The reason for timeout on Linux was a bug in wasi.PreopenList which I've patched up in #5829. I've rebased this PR on top that one, so hopefully the CI should be green again.

kubkon · 2020-08-12T22:03:07Z

I've decided to redo this PR in a major way such that:

I've now moved the concept of realpath relative to some file descriptor directly to std.fs.Dir structure since it seems to belong there better rather than in std.os which will be renamed to std.posix in the near future. This way, we can safely make this feature very host specific and so for instance FreeBSD is not supported due to the limitation of its kernel.
I've decided to forego WASI support for now until a solid use case comes by which would find a sandboxed version of realpath desirable where the returned path is always relative to the root file descriptor.
I've left std.os.getFdPath in place since it's a useful refactoring between std.os.realpath and std.fs.Dir.realpath; however, if anyone believes it shouldn't be there, I'll be happy to figure out a way of shuffling things around or removing it entirely.

@andrewrk would you care to take another look at this PR?

`std.os.getFdPath` is very platform-specific and can be used to query the OS for a canonical path to a file handle. Currently supported hosts are Linux, macOS and Windows. `std.fs.Dir.realpath` (and null-terminated, plus WTF16 versions) are similar to `std.os.realpath`, however, they resolve a path wrt to this `Dir` instance. If the input pathname argument turns out to be an absolute path, this function reverts to calling `realpath` on that pathname completely ignoring this `Dir`.

andrewrk

Looks good 👍

kubkon mentioned this pull request Jun 24, 2020

[libstd]: Enable CacheHash in WASI #5636

Closed

kubkon changed the title ~~Add std.os.realpathat and std.fs.Dir.realpath~~ [libstd]: add std.os.realpathat and std.fs.Dir.realpath Jun 24, 2020

kubkon mentioned this pull request Jun 24, 2020

Add tests for using directory operations on files #5684

Merged

kubkon force-pushed the dir-realpath branch from e951c74 to 55d4236 Compare June 25, 2020 07:07

kubkon force-pushed the dir-realpath branch from 55d4236 to 355b6b6 Compare June 26, 2020 04:52

squeek502 reviewed Jun 27, 2020

View reviewed changes

kubkon force-pushed the dir-realpath branch 2 times, most recently from d331b01 to 36aaff7 Compare June 27, 2020 13:49

joachimschmidt557 reviewed Jul 2, 2020

View reviewed changes

lib/std/fs.zig Outdated Show resolved Hide resolved

kubkon mentioned this pull request Jul 3, 2020

[libstd]: reworked CacheHash means its WASI compatible now #5784

Closed

kubkon force-pushed the dir-realpath branch from 1aacbf1 to 5b53746 Compare July 8, 2020 15:07

andrewrk reviewed Jul 8, 2020

View reviewed changes

kubkon force-pushed the dir-realpath branch from 5b53746 to 480676a Compare July 8, 2020 20:29

kubkon mentioned this pull request Jul 9, 2020

Fix incorrect continue condition in PreopeonList #5829

Merged

kubkon force-pushed the dir-realpath branch from 480676a to 389e564 Compare July 9, 2020 06:23

kubkon force-pushed the dir-realpath branch from 18c9535 to 02e9729 Compare July 13, 2020 06:22

kubkon force-pushed the dir-realpath branch from 02e9729 to 4b4eab4 Compare August 10, 2020 20:26

kubkon added the standard library This issue involves writing Zig code for the standard library. label Aug 10, 2020

kubkon force-pushed the dir-realpath branch from 4b4eab4 to 6f4e69a Compare August 11, 2020 19:06

kubkon marked this pull request as draft August 12, 2020 14:14

kubkon force-pushed the dir-realpath branch 2 times, most recently from f0a7510 to fbbd01a Compare August 12, 2020 21:54

kubkon changed the title ~~[libstd]: add std.os.realpathat and std.fs.Dir.realpath~~ Add std.os.getFdPath and std.fs.Dir.realpath Aug 12, 2020

kubkon added os-linux Linux os-macos macOS os-windows Microsoft Windows labels Aug 12, 2020

kubkon marked this pull request as ready for review August 12, 2020 22:03

kubkon requested a review from andrewrk August 12, 2020 22:03

kubkon force-pushed the dir-realpath branch from fbbd01a to dedffe4 Compare August 12, 2020 22:06

andrewrk approved these changes Aug 13, 2020

View reviewed changes

kubkon merged commit 3e2e6ba into ziglang:master Aug 13, 2020

kubkon deleted the dir-realpath branch August 13, 2020 05:08

kubkon mentioned this pull request Aug 13, 2020

'.' and '..' components in relative paths in std.os.windows.OpenFile #6044

Closed

	pub const realpath = os.realpath;
	pub const realpathZ = os.realpathZ;
	pub const realpathC = @compileError("deprecated: renamed to realpathZ");
	pub const realpathW = os.realpathW;

Uh oh!

Conversation

kubkon commented Jun 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andrewrk commented Jun 25, 2020

Uh oh!

kubkon commented Jun 25, 2020

Uh oh!

kubkon commented Jun 26, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

squeek502 Jun 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kubkon commented Jul 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andrewrk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kubkon commented Jul 8, 2020

Uh oh!

andrewrk commented Jul 8, 2020

Uh oh!

kubkon commented Jul 8, 2020

Uh oh!

andrewrk commented Jul 8, 2020

Uh oh!

kubkon commented Jul 8, 2020

Uh oh!

kubkon commented Jul 8, 2020

Uh oh!

kubkon commented Jul 9, 2020

Uh oh!

kubkon commented Aug 12, 2020

Uh oh!

andrewrk left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kubkon commented Jun 24, 2020 •

edited

Loading

squeek502 Jun 27, 2020 •

edited

Loading

kubkon commented Jul 4, 2020 •

edited

Loading