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

WIP/Discussion Permit ignoring entry permissions #207

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

rbtcollins
Copy link
Contributor

Sometimes the extractor will know exactly what permissions they
want, in which case having to re-chmod things just adds latency
to the operation. HPC file systems in particular appear to be
extra-ordinarily slow at metadata operations (~1-2ms per inode).

Sometimes the extractor will know exactly what permissions they
want, in which case having to re-chmod things just adds latency
to the operation. HPC file systems in particular appear to be
extra-ordinarily slow at metadata operations (~1-2ms per inode).
@rbtcollins
Copy link
Contributor Author

Hmm, @alexcrichton what do you think of combining this with setting the initial mode more closely to the final desired mode - that is, we can't drop the 'w' bit, but we can include the 'x' bit (and when desired S and s etc on creat() - like my aborted patch for Windows - which will allow us to entirely avoid a separate chmod for these HPC filesystems I think. I can do that as a separate patch (we'd want both features ultimately - I think the end state unpack logic in rustup should be something like

  entry.set_mode(rustup_heuristic(path));
  entry.set_ignore_mtime();
  entry.unpack_vfs(vfs.clone());

where the heuristic is the current rustup heuristic (is it in bin? does it already have and 'x' bit? is it a dir?), and the vfs object is to facilitate IO parallelisation.

@rbtcollins
Copy link
Contributor Author

rbtcollins commented May 28, 2019

Ok so looking at the structure more closely, that is obviously going to be ugly, instead I propose adding a struct for unpack options.

so entry.unpack_with(&path, &options)

and options can be used to factor all the unpack options off of Entry if we want to, but also hold things like 'use this custom mode when unpacking'. I'm happy to do this as a refactoring with compat concerns etc etc; the first thing though will be to test this with the minimum viable code - just storing the mode on Entry and overriding the header mode in the logic.

This is important for reducing syscalls on cluster filesystems
where metadata opertions like chmod can be as slow as 2ms each.

Rather than requiring users to chmod post extraction permit
selection of mode during extraction of individual entries.

This will be complementary to ignore_permissions once mode setting
at creat(2) is in place, as then, for writable files at least, the
correct file mode can be created in a single syscall.

Possibly ignore_permissions will not be needed, if we can thread the
right permission logic through the process - I'm working on that
still.
@rbtcollins rbtcollins changed the title Permit ignoring entry permissions WIP/Discussion Permit ignoring entry permissions May 28, 2019
@rbtcollins
Copy link
Contributor Author

hmmm, and why do I only now notice that entries implement Read :).

Obviously that lets me do all this as entirely special case in rustup.

I'm not sure where the dividing line should be here - please treat this as a discussion point!

@alexcrichton
Copy link
Owner

I'm personally fine adding more features to this crate so long as it doesn't muddy the workflow too much, and having some sort of options builder-like approach seems reasonable to me. It's intended though that you can of course build your own extraction externally if necessary without requiring usage of this crate! If something is pretty rustup-specific and not particularly generalizable then I think it makes sense to build externally, but if it's presumably useful in the default case (like for Cargo extracting tarballs) it seems reasonable to add here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants