search

mesalock-linux / mesabox

A collection of core system utilities written in Rust for Unix-like systems (and now Windows)
BSD 3-Clause "New" or "Revised" License
137 stars 19 forks source link

Drop actual_path? #33

Open Arcterus opened 6 years ago

Arcterus commented 6 years ago

I am considering removing util::actual_path() and just making all the utilities assume that the current working directory that we should operate in is the current working directory of the binary. The original point of util::actual_path() was to support the testing framework, but we won't need it for that once #24 is merged.

I can see it still being useful in some cases where mesabox is used as a library within another program as it allows operating in different directories without modifying the arguments or changing the host program's current working directory (which could be problematic if that program were to be multi-threaded). At the same time, removing util::actual_path() makes the utilities simpler and reduces the number of allocations we need to perform.

Arcterus commented 6 years ago

If no one has a preference/feels like it is useful, I am leaning towards dropping it as I'm not sure the added complexity is worth it.

Arcterus commented 6 years ago

Hit the wrong button.

Arcterus commented 6 years ago

One option might be to keep the file descriptor in UtilData rather than the name of the current directory and then use the *at functions (like openat) to access files (although we might need to keep the directory path as well to pass to crates we use). When changing directories we'd just have to replace the file descriptor (and path if we need to keep that as well).

This solution would help avoid the allocation issue while still maintaining the current functionality. The downsides are that writing utilities remains less ergonomic than normal binaries and that we'd need to either use nix/libc functions directory or use less vetted libraries (e.g. openat).

mssun commented 6 years ago

No preference for me. I think we can drop it to keep the the code clean and simple.

Arcterus commented 6 years ago

Only thing about this that needs consideration really is what to do with cd in sh. Currently, we try to avoid forking as much as possible, so we don't actually spawn real subshells if we can avoid doing so. The problem with that is that if the user changes the directory in one of the fake "subshells," the directory will changed outside of the "subshell."

My line of thought right now is that we should avoid forking until we see cd, in which case we fork and execute the rest of the commands for the current subshell in a real subshell. Doing so requires keeping track of whether we are in a fake "subshell" and ensuring we only fork when we haven't already done so for the current "subshell."

Another solution might be to change the directory and then change it back once the "subshell" exits, but this could cause issues if env::set_current_dir() fails after the current working directory has already been changed. This solution might also cause problems if we ever add multi-threading or other related features.

(Note that currently cd just doesn't worry about any of this and changes the directory no matter if it's in a subshell or not and it ignores actual_path(), so we will need to change it regardless of our solution).