Skip to content

Updating directory metadata with fs.Dir #12377

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

Open
OliverHallam opened this issue Aug 8, 2022 · 3 comments
Open

Updating directory metadata with fs.Dir #12377

OliverHallam opened this issue Aug 8, 2022 · 3 comments
Labels
os-windows standard library This issue involves writing Zig code for the standard library.
Milestone

Comments

@OliverHallam
Copy link

I've stumbled into an issue with the std.fs.Dir API.

My use case is that I'm writing a zip file extractor (as an exercise to learn the language), and as such I want to set the modified time of the files and directories I create from the metadata in the zip file. For files, I have std.fs.File.updateTimes which works excellently, but there is no equivalent for directories.

I investigated a bit further, and noted there is an existing API on std.fs.Dir that try to update metadata (fs.Dir.setPermissions), and this forwards onto fs.File.setPermissions. Given this already exists, and there are already methods to retrieve the file times for a directory (Dir.metadata()), I thought it would not be controversial to add Dir.updateTimes similarly, and was going to put a PR together to do this.

However, when I tried this discovered a slightly more insidious problem, and that is on Windows, in order to update directory metadata, the handle needs to be opened with the FILE_WRITE_ATTRIBUTES permission - this is never requested when opening a file handle. This means the existing function (std.fs.Dir.setPermission) as well as my new function will always fail when run on Windows machines.

I was easily able to reproduce this with the following test:

test "set directory permissions" {
    var dir = try std.fs.cwd().makeDir("temp", .{});
    defer dir.close();

    var permissions = (try dir.metadata()).permissions();
    permissions.setReadOnly(false);
    try dir.setPermissions(permissions);
}

On a Windows machine, this will always fail with an AccessDenied error.

To fix this we simply need to add the FILE_WRITE_ATTRIBUTES permission to the Dir.OpenFileW method (which fixed these issues when I tried locally), but the question is when should we do this?

  1. We could request attribute write permissions every time we open a directory - this is consistent with other platforms (e.g. openDirWasi always requests FD_FILESTAT_SET_TIMES when opening a directory). This is the simplest approach, however it seems this is a bit overly aggressive, and would likely cause problems opening directories on readonly filesystems. It's probably a bug in WASI that it does this.

  2. We could continue requesting read permission, but open a second handle with write permissions if we want to update the attributes. This to me feels quite antithetical to the Zig philosophy though, and I think we should be explicit about opening handles.

  3. We will need to explicitly request attribute write permissions when we open the directory. This would likely be done by adding a new bool to the OpenDirOptions to explicitly request permissions to update directory metadata when we open a file.

I am ready to submit a PR that does fix this (adding the new option to OpenDirOptions, and adding Dir.updateTimes), but I figured it was worth opening an issue to check this was the right approach - apologies if that's not the right workflow, I'm a first-time contributor.

@OliverHallam OliverHallam changed the title Updating directory metadata Updating directory metadata with fs.Dir Aug 8, 2022
@Vexu Vexu added standard library This issue involves writing Zig code for the standard library. os-windows labels Aug 18, 2022
@Vexu Vexu added this to the 0.11.0 milestone Aug 18, 2022
@nektro
Copy link
Contributor

nektro commented Sep 18, 2022

putting in my vote for option 3

@ominitay
Copy link
Contributor

When you fix the problem, please add a test case to std/fs/test.zig. For some reason, the test I wrote didn't catch this.

@squeek502
Copy link
Collaborator

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

No branches or pull requests

6 participants