new dir module #916

scottlamb · 2018-06-28T15:46:37Z

Fixes #915

This is a lower-level interface than std::fs::ReadDir. Notable differences:

can be opened from a file descriptor (as returned by openat, perhaps
before knowing if the path represents a file or directory). Uses
fdopendir for this, available on all Unix platforms as of
add fdopendir on macOS rust-lang/libc#1018.
implements AsRawFd, so it can be passed to fstat, openat, etc.
can be iterated through multiple times without closing and reopening the
file descriptor. Each iteration rewinds when finished.
returns entries for . (current directory) and .. (parent directory).
returns entries' names as a CStr (no allocation or conversion beyond
whatever libc does).

asomers

I can understand the need for such a module. I'll have to give some thought to the best name for it, though.

@Susurrus do you have any thoughts?

asomers · 2018-06-28T16:06:48Z

src/dir.rs

+
+impl Entry {
+    pub fn inode(&self) -> u64 {
+        self.0.d_ino


This field is called d_fileno on FreeBSD. I don't know about the other BSDs; you should check.

Fixed: now matches the std implementation's cfg stuff.

asomers · 2018-06-28T16:09:04Z

src/dir.rs

+    }
+}
+
+// `Dir` is not `Sync`, but it's not reference-counted either, so it can be safely passed from one


What makes it non-Sync ?

Hmm, I was under the impression it was only thread-safe when used with different DIR objects. It turns out I was wrong with readdir_r, but according to this glibc documentation, that call might be obsoleted, and readdir will be specified to work as I'd imagined.

I'm inclined to update the comment and leave it non-Sync. Opinions?

Makes sense.

asomers · 2018-06-28T16:10:59Z

src/dir.rs

+///!    * can be opened from a file descriptor (as returned by `openat`, perhaps before knowing
+///!      if the path represents a file or directory).
+///!    * implements `AsRawFd`, so it can be passed to `fstat`, `openat`, etc.
+///!    * can be iterated through multiple times without closing and reopening the file


How can it be iterated through multiple times if it only implemented IntoIterator and not Iterator?

&mut Directory implements IntoIterator, so you can get an iterator more than once. The test does this.

I could have Dir implement Iterator instead but you'd have to remember to rewind it.

Oh, I see. You implemented IntoIterator for &Dir, not Dir. I think it would be more idiomatic instead for Dir to have a iter() method, that returns an object which implemented Iterator.

asomers · 2018-06-28T16:14:16Z

src/dir.rs

+        match ptr::NonNull::new(unsafe { libc::fdopendir(fd) }) {
+            None => {
+                let e = Error::last();
+                unsafe { libc::close(fd) };


Dir takes ownership of the file descriptor, and closes it on Drop. Unfortunately, RawFd is defined as an alias, so there's no way to enforce that the caller doesn't hold onto a copy. But could you at least warn the caller in the documentation? Bad stuff will happen if he accesses fd outside of the Dir interface.

AsRawFd::as_raw_fd has such a warning:

This method does not pass ownership of the raw file descriptor to the caller. The descriptor is only guaranteed to be valid while the original object has not yet been destroyed.

but I'll add one to Dir's docs as well where I mention the AsRawFd implementation.

asomers · 2018-06-28T16:18:47Z

src/dir.rs

+    }
+}
+
+#[allow(missing_debug_implementations)]


Generally, an ugly derived Debug implementation is better than none at all, because none at all prevents derive from working on larger types composed with this one. Is there any reason not to derive it?

I suppose not. Done.

asomers · 2018-06-28T16:28:46Z

src/dir.rs

+
+/// A directory entry, similar to `std::fs::DirEntry`.
+/// Note that unlike the std version, this may represent the `.` or `..` entries.
+#[derive(Copy, Clone)]


How about Debug?

I manually implemented Debug, so that name would be shown as a CStr rather than a [::c_char; 256] or whatever.

asomers · 2018-06-28T16:30:38Z

src/dir.rs

+    pub fn inode(&self) -> u64 {
+        self.0.d_ino
+    }
+    pub fn name(&self) -> &ffi::CStr {


spaces between methods, please. Also, please document all public methods.

Should be named file_name to match std::fs::DirEntry

asomers · 2018-06-28T16:31:39Z

src/dir.rs

+    /// See platform `readdir(3)` or `dirent(5)` manpage for when the file type is known;
+    /// notably, some Linux filesystems don't implement this. The caller should use `stat` or
+    /// `fstat` if this returns `None`.
+    pub fn type_(&self) -> Option<Type> {


Since Entry is basically a replacement for std::fs::DirEntry, why not make the API as close as possible? This method should be named file_type, and should return an object supporting methods like those of std::fs::FileType.

Hmm. I'd like to preserve the ability to see exactly what the OS returned, for a couple reasons:

general educational value

you might want to use this to filter out uninteresting types without calling stat. If you decide to look more closely, you might want to do the stat in a different way (openat + fstat), and you might want to see the other fields without a second stat call.

My general impression was that nix is supposed to pretty closely match what the Unix systems offer, where the std library is more high-level. Does that sound right?

I made a similar decision to preserve the potentially-useful . and .. entries (you might want to call ino() on them, for example) where std filters it out.

That's reasonable. But I would still rather call this method file_type or filetype. The trailing underscore is weird and unprecedented.

asomers · 2018-06-28T16:34:50Z

src/dir.rs

+}
+
+impl Entry {
+    pub fn inode(&self) -> u64 {


Should be named ino to match std::fs::DirEntry

asomers · 2018-06-28T16:40:57Z

src/dir.rs

+
+impl PartialEq<Entry> for Entry {
+    fn eq(&self, other: &Entry) -> bool {
+        self.inode() == other.inode() && self.name() == other.name()


This method could give incorrect results for files opened from different file systems. Probably best just to compare name and the Dir object's file descriptor.

Hmm, honestly, comparing two Entrys is probably not that useful at all. I just have it here for my test. What would you think of removing it and do something else there?

Removing it would be reasonable. Is there a reason that Entry needs to implement PartialEq?

scottlamb · 2018-07-05T18:20:55Z

src/dir.rs

+///!    * returns entries for `.` (current directory) and `..` (parent directory).
+///!    * returns entries' names as a `CStr` (no allocation or conversion beyond whatever libc
+///!      does).
+pub struct Dir(ptr::NonNull<libc::DIR>);


This didn't go over well with the continuous build:

error[E0412]: cannot find type `NonNull` in module `ptr` --> src/dir.rs:23:21 | 23 | pub struct Dir(ptr::NonNull<libc::DIR>); | ^^^^^^^ not found in `ptr`

so I switched to *mut libc::DIR instead.

That's because NonNull only got added in Rust 1.25.0. Nix's minimum requirement is 1.20.0.

scottlamb · 2018-07-05T18:22:49Z

Pushed a new commit that should address most of these comments.

Thanks for the quick review! I appreciate it even though I wasn't able to respond for a while (got sick, yuck).

asomers

One more thing: it's not safe to have two Iter objects alive at the same time, because they'll affect each other's directory pointers. It may be better to implement Iterator directly on Dir, and add an explicit rewind method.

asomers · 2018-07-05T20:30:58Z

src/dir.rs

+use libc::{dirent, readdir_r};
+
+/// An open directory.
+///! This is a lower-level interface than `std::fs::ReadDir`. Notable differences:


Does this render correctly in Rust Doc? Normally ///! annotates the enclosing object, which in this case would be the file. To describe Dir instead, you should have all ///s, with a blank line after the first.

It did, but fixed anyway. Also adding some extra whitespace to the docstrings so the one-line extract would be less overwhelming.

asomers · 2018-07-05T20:32:17Z

src/dir.rs

+
+impl Entry {
+    /// Returns the inode number (`d_ino`) of the underlying `dirent`.
+    #[cfg(any(target_os = "macos",


alphabetical ordering, please.

asomers · 2018-07-05T20:36:09Z

src/dir.rs

+    /// Returns the inode number (`d_fileno`) of the underlying `dirent`.
+    #[cfg(any(target_os = "freebsd",
+              target_os = "openbsd",
+              target_os = "bitrig",


bitrig is dead. You can remove it. Also, the others should be alphabetically ordered. Does Nix support any OSes that can't implement ino? If not, you can simplify this with cfg_if, like this:

pub fn ino(&self) -> u64 { cfg_if! { if #[cfg(any(target_os = ...))] { self.0.d_ino as u64 } else { self.0.d_filenno as u64 } } }

Hmm, that's neat, but I'm having trouble getting it to work. Tried a couple variations. Any ideas?

pub fn ino(&self) -> u64 { cfg_if! { if #[cfg(any(target_os = "android", target_os = "emscripten", target_os = "fuchsia", target_os = "haiku", target_os = "ios", target_os = "l4re", target_os = "linux", target_os = "macos", target_os = "solaris"))] { self.0.d_ino as u64 } else { self.0.d_fileno as u64 } } }

produces

error: expected one of `!` or `::`, found `.` --> src/dir.rs:158:21 | 158 | self.0.d_ino as u64 | ^ expected one of `!` or `::` here

and

cfg_if! { if #[cfg(any(target_os = "android", target_os = "emscripten", target_os = "fuchsia", target_os = "haiku", target_os = "ios", target_os = "l4re", target_os = "linux", target_os = "macos", target_os = "solaris"))] { pub fn ino(&self) -> u64 { self.0.d_ino as u64 } } else { pub fn ino(&self) -> u64 { self.0.d_fileno as u64 } } }

produces a bunch of errors, starting with:

error: expected one of `::` or `:`, found `)` --> src/dir.rs:175:29 | 175 | pub fn ino(&self) -> u64 { self.0.d_ino as u64 } | ^ expected one of `::` or `:` here

Looks like cfg_if doesn't work with expressions, only items. An item is something like a module or a function.
https://docs.rs/cfg-if/0.1.4/cfg_if/macro.cfg_if.html
https://doc.rust-lang.org/reference/items.html

Hmm, function didn't work either (second try above). I got it to work by having a separate impl for this vs the other stuff, but then the two impls are separate in the documentation, which is annoying.

I think I'm giving up on cfg_if! here, but I can do #[cfg(any(target_os = "bar", target_os="foo"))] vs #[cfg(not(any(target_os = "bar", target_os="foo")))] so it's obviously comprehensive.

asomers · 2018-07-05T20:37:38Z

src/dir.rs

+    /// See platform `readdir(3)` or `dirent(5)` manpage for when the file type is known;
+    /// notably, some Linux filesystems don't implement this. The caller should use `stat` or
+    /// `fstat` if this returns `None`.
+    pub fn type_(&self) -> Option<Type> {


That's reasonable. But I would still rather call this method file_type or filetype. The trailing underscore is weird and unprecedented.

asomers · 2018-07-05T20:38:47Z

test/test_dir.rs

+use std::fs::File;
+use self::tempdir::TempDir;
+
+fn open(path: &::std::path::Path) -> nix::Result<Dir> {


Would it be worthwhile to make this a method of Dir, so that you can open by either RawFd or a path?

Done - both fcntl::open and fcntl::openat variants. Unlike my quick test version, I'm using a NixPath, and I'm expecting the caller to provide OFlags, so they can do O_NOFOLLOW or O_EXLOCK or whatever as desired.

asomers · 2018-07-05T20:42:43Z

test/test_dir.rs

+        .map(|e| e.file_name().to_str().unwrap().to_owned())
+        .collect();
+    assert_eq!(&entry_names[..], &[".", "..", "bar", "foo"]);
+    assert!(&[Some(Type::Directory), None].contains(&entries[0].type_())); // .: dir


It would be worthwhile adding a comment explaining why the check for None is necessary.

scottlamb · 2018-07-05T22:00:33Z

One more thing: it's not safe to have two Iter objects alive at the same time, because they'll affect each other's directory pointers. It may be better to implement Iterator directly on Dir, and add an explicit rewind method.

The lifetime should prevent that problem. Each Iter has a &'d mut Dir, and there can only be one of those at once.

asomers

Ahh, I didn't notice that iter takes a mutable reference. That should work.

asomers · 2018-07-05T22:35:42Z

src/dir.rs

+///    * can be opened from a file descriptor (as returned by `openat`, perhaps before knowing
+///      if the path represents a file or directory).
+///    * implements `AsRawFd`, so it can be passed to `fstat`, `openat`, etc.
+///      The descriptor continues to be owned by the `Dir`, so callers must not keep a `RawFd`


s/descriptor/file/

file descriptor? it's really not the file.

Ok, I see your point. Do you think it would make sense to implement AsRef<RawFd> instead of AsRawFd? That way the user would be unable to keep the RawFd if the drop the Dir.

Hmm, I think this argument applies equally well to anywhere else that uses AsRawFd, and Google doesn't find any results for AsRef<RawFd>. I think I'd prefer to match the rest of the ecosystem.

and RawFd is Copy, so AsRef<RawFd> is probably just as easy to mess up anyway.

I forgot about Copy. In that case you're right that AsRef<RawRd> wouldn't have much advantage.

asomers · 2018-07-05T22:41:03Z

src/dir.rs

+    }
+
+    /// Opens the given path as with `fcntl::openat`.
+    pub fn openat<P: ?Sized + NixPath>(dirfd: RawFd, path: &P, oflag: OFlag) -> Result<Self> {


hardcoding Mode::empty prevents this API from being used to create new directories. Any reason not to expose the mode argument of fcntl::openat and fcntl::open?

Done. I just never imagined using this to create directories would actually work, but it does on Linux. (On macOS, it returns EINVAL.)

Any idea why? Can openat not create directories on OSX?

Susurrus

Overall looks pretty good I think. A few minor style changes here. But I think you and asomers have hashed out most of it.

Susurrus · 2018-08-06T00:15:54Z

src/dir.rs

+            libc::DT_REG => Some(Type::File),
+            libc::DT_LNK => Some(Type::Symlink),
+            libc::DT_SOCK => Some(Type::Socket),
+            /* libc::DT_UNKNOWN | */ _ => None,


Wouldn't this be better to actually use DT_UNKNOWN and then have another catchall that maps to unreachable!()?

If the list of DT_ types is incomplete (which seems possible—I haven't read all the platforms' manpages, and a platform could add one anyway), what should this code do? With unreachable!, it'd panic. I'd prefer it return None. Calling code is supposed to handle DT_UNKNOWN gracefully, and it makes sense to me to handle an unexpected enum type in the same way.

Susurrus · 2018-08-06T00:18:31Z

test/test_dir.rs

+
+#[test]
+fn rewind() {
+    let tmp = TempDir::new("nix-dir-rewind").unwrap();


Can't we have anonymous folders? I'd prefer we use that style rather than naming everything.

The tempdir crate doesn't seem to support that. tempfile version 3 does (and that obsoletes tempdir), but nix still seems to be on version 2. This call to TempDir::new matches the other ones in the tests dir.

Susurrus · 2018-08-06T00:18:34Z

test/test_dir.rs

+
+#[test]
+fn read() {
+    let tmp = TempDir::new("nix-dir-read").unwrap();


Can't we have anonymous folders? I'd prefer we use that style rather than naming everything.

Susurrus · 2018-08-06T00:19:32Z

test/test_dir.rs

+        .collect();
+    assert_eq!(&entry_names[..], &[".", "..", "bar", "foo"]);
+
+    // Check file types. The system is allowed to return DT_UNKNOWN (aka None here) but if it does


But why would it return DT_UNKNOWN? Does it do this in practice on any systems?

Yes, it does. From the Linux readdir(3) manpage:

Currently, only some filesystems (among them: Btrfs, ext2, ext3, and ext4) have full support for returning the file type in d_type. All applications must properly handle a return of DT_UNKNOWN.

Susurrus · 2018-08-06T00:20:19Z

src/dir.rs

+
+impl Entry {
+    /// Returns the inode number (`d_ino`) of the underlying `dirent`.
+    #[cfg(any(target_os = "android",


Can we use cfg-if here instead of these huge if/else blocks?

I tried this on July 5th, and the only way I could get it to work was by having two impl blocks, which makes the rustdoc output look weird. cfg_if can only be attached at the "item" level; apparently neither functions nor expressions are items.

Susurrus · 2018-08-06T00:23:12Z

src/dir.rs

+use std::{ffi, fmt, ptr};
+use sys;
+
+#[cfg(target_os = "linux")]


Can we use the cfg_if! macro here instead? It's a little more verbose but I find it easier to read an if-else rather than trying to determine that through consecutive cfg statements.

scottlamb · 2018-08-29T04:48:39Z

Anything else I need to do to finish this?

Susurrus · 2018-08-31T03:37:46Z

LGTM. @asomers any last changes you think are necessary?

asomers · 2018-08-31T13:18:50Z

No changes except a squash.

scottlamb · 2018-08-31T14:07:37Z

Squashed. Thank you!

asomers · 2018-08-31T14:27:27Z

bors r+

916: new dir module r=asomers a=scottlamb Fixes #915 This is a lower-level interface than `std::fs::ReadDir`. Notable differences: * can be opened from a file descriptor (as returned by `openat`, perhaps before knowing if the path represents a file or directory). Uses `fdopendir` for this, available on all Unix platforms as of rust-lang/libc#1018. * implements `AsRawFd`, so it can be passed to `fstat`, `openat`, etc. * can be iterated through multiple times without closing and reopening the file descriptor. Each iteration rewinds when finished. * returns entries for `.` (current directory) and `..` (parent directory). * returns entries' names as a `CStr` (no allocation or conversion beyond whatever libc does). Co-authored-by: Scott Lamb <slamb@slamb.org>

bors · 2018-08-31T14:32:50Z

Build failed

buildbot/nix-rust/nix amd64_fbsd11

asomers · 2018-08-31T14:46:39Z

Looks like we need to merge PR #900 first.

Susurrus · 2018-09-02T15:45:12Z

#900 was just merged

bors r+

916: new dir module r=Susurrus a=scottlamb Fixes #915 This is a lower-level interface than `std::fs::ReadDir`. Notable differences: * can be opened from a file descriptor (as returned by `openat`, perhaps before knowing if the path represents a file or directory). Uses `fdopendir` for this, available on all Unix platforms as of rust-lang/libc#1018. * implements `AsRawFd`, so it can be passed to `fstat`, `openat`, etc. * can be iterated through multiple times without closing and reopening the file descriptor. Each iteration rewinds when finished. * returns entries for `.` (current directory) and `..` (parent directory). * returns entries' names as a `CStr` (no allocation or conversion beyond whatever libc does). Co-authored-by: Scott Lamb <slamb@slamb.org>

bors · 2018-09-02T15:48:02Z

Build failed

buildbot/nix-rust/nix amd64_fbsd11

asomers · 2018-09-02T16:17:54Z

Looks like you need to rebase and replace the tempdir crate with tempfile.

scottlamb · 2018-09-03T23:57:45Z

Rebased, replaced tempfile with tempdir.

Susurrus · 2018-09-04T00:01:20Z

Can you add an Added entry into the CHANGELOG? Then I think this LGTM.

This is a lower-level interface than `std::fs::ReadDir`. Notable differences: * can be opened from a file descriptor (as returned by `openat`, perhaps before knowing if the path represents a file or directory). Uses `fdopendir` for this, available on all Unix platforms as of rust-lang/libc#1018. * implements `AsRawFd`, so it can be passed to `fstat`, `openat`, etc. * can be iterated through multiple times without closing and reopening the file descriptor. Each iteration rewinds when finished. * returns entries for `.` (current directory) and `..` (parent directory). * returns entries' names as a `CStr` (no allocation or conversion beyond whatever libc does).

scottlamb · 2018-09-04T00:05:24Z

Done

Susurrus · 2018-09-04T00:07:32Z

Awesome! Thanks @scottlamb!

bors r+

916: new dir module r=Susurrus a=scottlamb Fixes #915 This is a lower-level interface than `std::fs::ReadDir`. Notable differences: * can be opened from a file descriptor (as returned by `openat`, perhaps before knowing if the path represents a file or directory). Uses `fdopendir` for this, available on all Unix platforms as of rust-lang/libc#1018. * implements `AsRawFd`, so it can be passed to `fstat`, `openat`, etc. * can be iterated through multiple times without closing and reopening the file descriptor. Each iteration rewinds when finished. * returns entries for `.` (current directory) and `..` (parent directory). * returns entries' names as a `CStr` (no allocation or conversion beyond whatever libc does). Co-authored-by: Scott Lamb <slamb@slamb.org>

bors · 2018-09-04T00:54:04Z

Build succeeded

asomers requested changes Jun 28, 2018

View reviewed changes

scottlamb commented Jul 5, 2018

View reviewed changes

asomers requested changes Jul 5, 2018

View reviewed changes

Susurrus suggested changes Aug 6, 2018

View reviewed changes

scottlamb mentioned this pull request Aug 6, 2018

directory tree support scottlamb/http-serve#7

Open

Susurrus approved these changes Aug 31, 2018

View reviewed changes

asomers approved these changes Aug 31, 2018

View reviewed changes

scottlamb force-pushed the pr-dir branch from 370ec52 to 6d6e9c5 Compare August 31, 2018 14:07

scottlamb force-pushed the pr-dir branch from 6d6e9c5 to 632b00c Compare September 3, 2018 23:56

scottlamb force-pushed the pr-dir branch from 632b00c to f9ebcb7 Compare September 4, 2018 00:05

bors bot merged commit f9ebcb7 into nix-rust:master Sep 4, 2018

new dir module #916

new dir module #916

Conversation

scottlamb commented Jun 28, 2018

asomers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottlamb commented Jul 5, 2018

asomers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottlamb commented Jul 5, 2018

asomers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Susurrus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottlamb commented Aug 29, 2018

Susurrus commented Aug 31, 2018

asomers commented Aug 31, 2018

scottlamb commented Aug 31, 2018

asomers commented Aug 31, 2018