Add xdg_basedir API#157518
Conversation
rustbot
added
O-unix
|
r? @aapoalas rustbot has assigned @aapoalas. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
cc @BurntSushi (+1 on ACP) |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Might be interesting to @rami3l. |
|
@Kobzol Thanks for the ping! Our current plan with @Cloud0310 regarding rust-lang/rustup#247 is slightly more complicated than this because it involves handling both old and new |
There was a problem hiding this comment.
Looks good to me - I had a bunch of comments here and there but nothing that I'd consider blocking.
Let me know your thoughts on the comments and one we've handled them one way or another (made changes or decided to ignore) then we can put this on the train.
| @@ -0,0 +1,166 @@ | |||
| //! XDG (X Desktop Group) related functionality for Unix platforms. | |||
| //! | |||
| //! The [XDG Base Directory Specification][basedir] defines where user-specific | |||
There was a problem hiding this comment.
suggestion: I'd reorder this:
| //! The [XDG Base Directory Specification][basedir] defines where user-specific | |
| //! The [XDG Base Directory Specification][basedir] defines a set of base directories, relative to which user-specific files should be looked for. |
| //! | ||
| //! Directories returned by this module are not guaranteed to exist yet. If the | ||
| //! directory does not exist, an application should attempt to create it with | ||
| //! [permissions mode][super::fs::PermissionsExt::from_mode] `0o700`. |
There was a problem hiding this comment.
thought: Not the responsibility of this module at all, but this smells like the start of a TOCTOU bug story.
| use crate::path::{Path, PathBuf}; | ||
|
|
||
| fn xdg_home_dir() -> PathBuf { | ||
| // Note: home_dir can return `Some("")` in some cases. We assume that in |
There was a problem hiding this comment.
suggestion: Maybe mention the getpwuid_r function by name for the "in some cases" - I assume that's the main / only case referred to here?
| } | ||
| } | ||
|
|
||
| fn xdg_dir(env: &str, fallback_home_subdir: impl AsRef<Path>) -> PathBuf { |
There was a problem hiding this comment.
nitpick: Only ever called with fallback_home_subdir: &'static str - could just monomorphise this in code to save that little bit of compile time.
| /// An application `appid` would typically be expected to write its state data to | ||
| /// `{state_home_dir}/{appid}/**/*`. | ||
| /// | ||
| /// Common kinds of state data include actions history (such as logs, history, |
There was a problem hiding this comment.
praise: Big fan of explaining the purpose a little <3
| xdg_dir("XDG_STATE_HOME", ".local/state") | ||
| } | ||
|
|
||
| /// A base directory relative to which user-specific non-essential caches should be written. |
There was a problem hiding this comment.
nitpick: Why the rewording from the XDG specification's "non-essential (cached) data" to "non-essential caches"?
| pub struct XdgDirsIter(Option<OsString>); | ||
|
|
||
| impl XdgDirsIter { | ||
| fn new(env: &str, default: impl AsRef<OsStr>) -> Self { |
There was a problem hiding this comment.
nitpick: Here too the default is always &'static str - could just monomorphise manually.
|
|
||
| fn next(&mut self) -> Option<Self::Item> { | ||
| let dirs = self.0.take()?; | ||
| let next = split_paths(&dirs).next()?; |
There was a problem hiding this comment.
thought: So we're effectively reproducing a split_paths iterator by using split_paths itself combined with manual byte shifting.
This... definitely isn't that great. It's a little sad that we perform PathBuf copies "ahead of time" per item but that kind of goes with not having lending iterators. Draining the OsString one set of bytes at a time though... that's really sad indeed.
I don't think it's necessarily something that should block this PR, but this really could maybe use a little more thinking / polish. Like, maybe we should store the current position in the XdgDirsIter and slice &dirs before calling split_paths to avoid the draining? We'd need an unsafe for the slicing, but its safety condition is exactly the same one we have in the loop right now, and we wouldn't need any other unsafe beyond that.
6 participants
Implements an API providing easy access to the XDG Base Directories.
XdgDirsIterisn't as performant as it could be due to needing to shift things forward, but this was the simplest thing I could come up with that wasn't just entirely reimplementingsplit_pathswith owned input.I chose to defer
runtime_dir(XDG_RUNTIME_DIR) since the lack of a specified default (but specified expectation for applications to determine an alternative option somehow) makes the correct usage (and thus ideal API) less obvious.std::os::unix::xdglibs-team#805xdg_basedir#157515