add support for dirent related function #558

Mic92 · 2017-03-17T15:51:47Z

This pr is currently work in progress, as it depends on an outstanding pull request for fdopendir in libc. I wanted to know if this function and DIR wrapper I added fit into the scope of the nix crate.

Mic92 · 2017-03-17T15:53:21Z

src/dirent.rs

+}
+
+impl Dir {
+    pub fn read(&mut self) -> Result<Option<&libc::dirent>> {


borrowing mutually Dir is required here as the dirent reference might get overwritten by subsequent calls.

Susurrus · 2017-03-17T17:55:41Z

src/dirent.rs

+use errno;
+
+pub struct Dir {
+    pub handle: *mut DIR,


I don't think this field should be public. Instead I'd propose implementing AsRawFd and IntoRawFd instead.

Maybe Into<*mut DIR> because AsRawFd is for file descriptor.

Sorry, you're right. Into<*mut DIR> is what you'd want to do.

There is also dirfd which you could use to implement the *RawFd traits if you wanted to.

I currently leave it private as I have implemented all functions, where the dir pointer is useful. If it turns out, that I missed a use case, it can be still added.

kamalmarhubi

Thanks for this! While here, it seems like you should also add opendir(3). We'd also need something in the changelog.

However, I'm trying to decide on what the best way to expose this is. At one end, we could have fdopendir(3) and opendir(3) as here, and instead of Dir::read, we have readdir as a free function. At the other end, we expose readdir(3) as an iterator. And somewhere in between is the approach here.

Thoughts @fiveop @posborne?

kamalmarhubi · 2017-03-19T07:13:16Z

Cargo.toml

@@ -21,7 +21,7 @@ preadv_pwritev = []
 signalfd = []

 [dependencies]
-libc = { git = "https://github.com/rust-lang/libc" }
+libc = { git = "https://github.com/Mic92/libc", branch = "master" }


Will wait till this hits libc upstream.

kamalmarhubi · 2017-03-19T07:16:53Z

src/dirent.rs

+                _ => Err(Error::last().into()),
+            }
+        } else {
+            Ok(Some(unsafe{&*dirent}))


Could you annotate the lifetime in the signature so we're sure that this reference has the correct lifetime:

pub fn read<'a>(&'a mut self) -> Result<Option<&'a libc::dirent>>

I switched back to readdir

Mic92 · 2017-03-20T12:42:55Z

added seekdir/telldir

kamalmarhubi · 2017-03-20T14:37:31Z

src/dirent.rs

+    };
+    if dirent.is_null() {
+        match Errno::last() {
+            errno::UnknownErrno => Ok(None),


Is this right? It seems that on error, -1 is returned, not NULL. NULL indicates end of stream.

from readdir(3):

If the end of the directory stream is reached, NULL is returned and errno is not changed. If an error occurs, NULL is returned and errno is set appropriately. To distinguish end of stream and from an error, set errno to zero before calling readdir() and then check the value of errno if NULL is returned.

Right you are! I must have had one of the other functions' man pages open.

kamalmarhubi

Thanks ofr the update! If you're adding the whole family, could you also include closedir(3), rewinddir(3), scandir(3), and dirfd(3)? Either that or update the commit message.

It would also make sense to implement AsRawFd and IntoRawFd for Dir using dirfd(3), but I'm happy to have that in a later PR.

kamalmarhubi · 2017-03-20T14:46:17Z

Ah just noted that @Susurrus had mentioned {As,Into}RawFd already. Oh and there's also the changelog entry! :-)

Mic92 · 2017-03-20T21:28:53Z

I would leave adding the other functions open to someone else, as I currently only add functions, I need to proceed my project.

kamalmarhubi · 2017-03-20T22:17:00Z

@Mic92 that's fair. Could you look at the CI failures? They seem legitimate.

Mic92 · 2017-03-20T22:28:02Z

Order of .. and . seems to differ depending on platform

Mic92 · 2017-03-22T10:34:53Z

src/dirent.rs

+    }
+}
+
+pub fn readdir<'a>(dir: &'a mut Dir) -> Result<Option<&'a libc::dirent>> {


To obtain d_name I use:

let name = OsStr::from_bytes(unsafe { &mem::transmute::<[i8; 256], [u8; 256]>(entry.d_name) });

any idea how I could how add a convenience wrapper to this library?

Mic92 · 2017-04-08T08:49:28Z

This is ready for review.

homu · 2017-04-15T21:12:53Z

☔ The latest upstream changes (presumably #536) made this pull request unmergeable. Please resolve the merge conflicts.

homu · 2017-05-17T04:23:17Z

☔ The latest upstream changes (presumably ba5c837) made this pull request unmergeable. Please resolve the merge conflicts.

Susurrus · 2017-05-17T04:44:00Z

@Mic92 Can you add comments for every function based on their man pages? We're trying to have complete documentation coverage for all PRs moving forward. You can see where things are lacking when you run cargo doc --no-deps --open

Susurrus · 2017-05-17T04:45:17Z

src/dirent.rs

+#[cfg(not(any(target_os = "ios", target_os = "macos")))]
+use std::os::unix::io::RawFd;
+
+pub struct Dir(*mut DIR);


Maybe name this DirStream so it's a little more clear?

I prefer Dir as it it closer to the underlying API and matches the function names better, which all end in dir.

Mic92 · 2017-07-07T00:22:21Z

depends on rust-lang/libc#647

Susurrus · 2017-07-07T04:02:01Z

test/test_dirent.rs

+use std::path::Path;
+use tempdir::TempDir;
+
+fn test_readdir<OPEN>(open_fn: OPEN)


Please don't use all-caps here for a type name. Maybe just use O.

Susurrus · 2017-07-07T04:03:16Z

test/test_dirent.rs

+    assert_eq!(letter1, '.' as u8);
+
+    let pos = telldir(&mut dir);
+    seekdir(&mut dir, pos); // no-op


Could we have a test where we seek back a directory and check that the same directory is read twice (so that we no seekdir worked? I'd like to have a non-nop test of it.

Susurrus · 2017-07-07T04:05:09Z

src/dirent.rs

+    }
+}
+
+/// Opens a directory stream corresponding to the directory name.


Can you run cargo doc --no-deps --open locally with this code and make sure the first sentence shows up nicely on the page listing all the functions here? I'm certain that some of these sentences are too long and that you need an empty /// on the next line to make them show up nicely.

Susurrus · 2017-07-07T04:05:21Z

src/dirent.rs

+    }
+
+    /// Inode number
+    pub fn ino(&self) -> libc::ino_t {


Any reason not to call this inode here for a little more clarity?

Susurrus · 2017-07-07T04:05:41Z

src/dirent.rs

+#[cfg(not(any(target_os = "ios", target_os = "macos")))]
+use std::os::unix::io::RawFd;
+
+/// Directory Stream object


Why don't we call this DirectoryStream or DirStream instead of Dir?

also renamed Dirent to DirectoryEntry for consistency.

Susurrus · 2017-07-07T04:06:44Z

This is looking pretty good. Mostly some minor changes here. I would love to have more comprehensive module documentation, but I think linking to the man pages should be fine.

Susurrus · 2017-07-07T15:40:15Z

src/dirent.rs

+/// Consumes directory stream and return underlying directory pointer.
+///
+/// The pointer must be deallocated manually using `libc::closedir`
+impl Into<*mut DIR> for DirectoryStream {


A convention in std is to use a method named into_raw here when you're unwrapping a type. I'd suggest using that here instead of Into<*mut DIR>. I don't see this used anywhere, however. Can you either remove it if it's unnecessary, or if it's necessary add it to the test or use it in an example.

Susurrus · 2017-07-07T15:44:25Z

src/dirent.rs

+///
+/// The `loc` argument should be a value returned by a previous call to `telldir`
+#[cfg(not(any(target_os = "android")))]
+pub fn seekdir<'a>(dir: &'a mut DirectoryStream, loc: c_long) {


I'm thinking a newtype around c_long for use as the return value of telldir and the 2nd argument of seekdir would be good, since that's an invariant that should be embedded in the type system. I can't think of a good name for it, hopefully you can, but I think we should definitely do it.

Susurrus · 2017-07-07T15:45:17Z

src/dirent.rs

+    }
+}
+
+/// Like `opendir()`, but returns a directory stream for the directory


Please make the first line all one line. You can go out to 99 characters with your comments.

Susurrus · 2017-07-07T15:48:13Z

test/test_dirent.rs

+                       .unwrap()
+                       .name()
+                       .to_bytes()[0];
+    assert_eq!(letter3, '.' as u8);


This would be more clear if you compared letter2 and letter3 directly if that's what should be happening here.

Susurrus · 2017-07-07T15:53:10Z

test/test_dirent.rs

+
+fn test_readdir<Open>(open_fn: Open)
+    where Open: Fn(&Path) -> DirectoryStream
+{


Can you check the full name of the directory? It's confusing that you're checking just the first character of the filename. It'd also be helpful if you created a directory in the tempdir you made so you have 3 directories, then you iterate through all of them checking their names, and then seek back to check that seeking works.

There is no guarantee on the order of directory entries.

I still think it's worth checking their names, we can treat it as a set, where the order doesn't matter but the contents should still be the same. We'd expect to encounter both "." and "..", but we aren't certain of the order, but that's fine, we can still do that check.

Susurrus · 2017-07-07T15:54:49Z

Just saw you comment about the order of . and .. varying depending on platform. I do know many platforms don't guarantee returning things in alphabetical order. Maybe you should iterate through all the directories collecting their names into a sorted list and then compare that?

Susurrus · 2017-08-16T20:54:41Z

@Mic92 I'd still like to get this merged, can you resolve the lingering issues and rebase this?

Mic92 · 2017-08-18T06:35:10Z

Next week - this week I am on holiday. Maybe CHANGELOG.md should not be modified by pull requests at all. It creates to much merge conflicts and could be easily added from the pull request description after a merge.

Mic92 · 2017-09-01T18:49:35Z

Can you restart the one failing test? Looks like a transient failure: https://travis-ci.org/nix-rust/nix/builds/270775924

asomers · 2017-09-01T19:16:52Z

offending test restarted

Mic92 · 2017-09-06T09:40:56Z

if somebody is interested in dirfd and wants to make a pull request, feel free to cherry-pick from this branch: https://github.com/Mic92/nix/tree/dirfd

Susurrus · 2017-11-12T23:04:57Z

Looks like there are legitimate test failures here. @Mic92 Are you interested in pushing this PR through to be merged? Based on your last comment, it sounds like you aren't interested in continuing to work on this PR. Is that correct?

Mic92 · 2017-11-13T11:17:33Z

Yes.

Mic92 commented Mar 17, 2017

View reviewed changes

Susurrus reviewed Mar 17, 2017

View reviewed changes

kamalmarhubi reviewed Mar 19, 2017

View reviewed changes

Mic92 force-pushed the fdopendir branch 3 times, most recently from 1eb7cb7 to e498677 Compare March 20, 2017 12:42

Mic92 force-pushed the fdopendir branch 2 times, most recently from 81ffa22 to 3ca111b Compare March 20, 2017 13:30

kamalmarhubi reviewed Mar 20, 2017

View reviewed changes

Mic92 changed the title ~~add support for fdopendir~~ add support for dirent related function Mar 20, 2017

Mic92 force-pushed the fdopendir branch from 3ca111b to 75124f5 Compare March 20, 2017 22:27

Mic92 force-pushed the fdopendir branch 6 times, most recently from d22cfb1 to 0cad72e Compare March 21, 2017 00:35

Mic92 commented Mar 22, 2017

View reviewed changes

Mic92 force-pushed the fdopendir branch from 0cad72e to ff64bdb Compare March 26, 2017 10:36

Mic92 force-pushed the fdopendir branch from ff64bdb to 8e486f4 Compare April 8, 2017 08:47

Susurrus reviewed May 17, 2017

View reviewed changes

Mic92 force-pushed the fdopendir branch 2 times, most recently from 39cc8fd to 338d169 Compare July 6, 2017 23:58

Susurrus suggested changes Jul 7, 2017

View reviewed changes

Mic92 force-pushed the fdopendir branch 2 times, most recently from 8aa9e48 to 6ed79b0 Compare July 7, 2017 07:16

Susurrus suggested changes Jul 7, 2017

View reviewed changes

Mic92 force-pushed the fdopendir branch from 6ed79b0 to 2a75d70 Compare July 7, 2017 15:53

Mic92 force-pushed the fdopendir branch 4 times, most recently from db5380b to 1c570ea Compare July 7, 2017 21:28

Mic92 force-pushed the fdopendir branch 2 times, most recently from ce662b6 to 6035dd8 Compare September 1, 2017 10:28

Mic92 changed the title ~~add support for dirent related function~~ [WIP] add support for dirent related function Sep 1, 2017

Mic92 force-pushed the fdopendir branch from 6035dd8 to c78f2b2 Compare September 1, 2017 11:22

Mic92 changed the title ~~[WIP] add support for dirent related function~~ add support for dirent related function Sep 1, 2017

add support for dirent family

a8c2dd6

Mic92 force-pushed the fdopendir branch from c78f2b2 to a8c2dd6 Compare September 4, 2017 09:12

Mic92 closed this Nov 13, 2017

add support for dirent related function #558

add support for dirent related function #558

Conversation

Mic92 commented Mar 17, 2017 • edited Loading

Mic92 Mar 17, 2017 • edited Loading

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

kamalmarhubi left a comment • edited Loading

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

Mic92 commented Mar 20, 2017 • edited Loading

kamalmarhubi Mar 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kamalmarhubi left a comment

Choose a reason for hiding this comment

kamalmarhubi commented Mar 20, 2017

Mic92 commented Mar 20, 2017

kamalmarhubi commented Mar 20, 2017

Mic92 commented Mar 20, 2017

Choose a reason for hiding this comment

Mic92 commented Apr 8, 2017

homu commented Apr 15, 2017

homu commented May 17, 2017

Susurrus commented May 17, 2017

Choose a reason for hiding this comment

Mic92 Jul 6, 2017 • edited Loading

Choose a reason for hiding this comment

Mic92 commented Jul 7, 2017

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 commented Jul 7, 2017

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 commented Jul 7, 2017

Susurrus commented Aug 16, 2017

Mic92 commented Aug 18, 2017

Mic92 commented Sep 1, 2017 • edited Loading

asomers commented Sep 1, 2017

Mic92 commented Sep 6, 2017

Susurrus commented Nov 12, 2017

Mic92 commented Nov 13, 2017

Mic92 commented Mar 17, 2017 •

edited

Loading

Mic92 Mar 17, 2017 •

edited

Loading

kamalmarhubi left a comment •

edited

Loading

Mic92 commented Mar 20, 2017 •

edited

Loading

kamalmarhubi Mar 20, 2017 •

edited

Loading

Mic92 Jul 6, 2017 •

edited

Loading

Mic92 commented Sep 1, 2017 •

edited

Loading