Skip to content
This repository has been archived by the owner on May 4, 2018. It is now read-only.

fs: uv_fs_{open,read,close}_dir #1574

Closed
wants to merge 1 commit into from

Conversation

misterdjules
Copy link

Tested on Linux, MacOS X, SmartOS and Windows.

Fixes #1430.

@misterdjules
Copy link
Author

@saghul @tjfontaine Reopening a new PR because GitHub wouldn't let me reopen #1521.

The code has been updated to follow @saghul's design closely.

There are still a few of things that are unclear or missing:

  • As @saghul mentioned: Regardless of the API accepting a single entry or an array of them, space for the name needs to be allocated, maybe have an extra function to help with that?. Do we want something similar to handles' allocation callback?
  • Regarding @piscisaureus's comment about GetFileAttributesW/FindFirstFileW being inherently racey, using only FindFirstFileW on an entry that is not a directory would return UV_ENOENT, which I find a bit counter-intuitive and inconsistent. @piscisaureus: before looking into the approach you recommended ("Open the directory, Use GetFileInformationByHandle to read the attributes. Bail out and close if it's not a directory, otherwise start enumerating entries (using a different call).", I'd like to make sure I understood it correctly. Do you mean that we could use CreateFile first, then GetFileInformationByHandle to check if it's a directory, and then use lower-level APIs (not FindFirstFile/FindNextFile) to iterate through all entries of the HANDLE returned by CreateFile? It seems to be quite more complicated than using FindFirstFile/FindNextFile, but it could be because I'm not familiar with these others lower-level APIs.
  • Regarding the following question: "What happens if we create 2 readdir requests for the same directory? On Unix we should be fine because we'd use readdir_r, but how about windows?", I've done some research to determine how to answer this question, but I'm not done yet.

Currently, all tests pass on all supported platforms. I've also done a lot of tests with directories containing more than 1 million entries, and the memory footprint stays constant and low.

Hopefully this is a step in the right direction!

Thank you,

*
* struct dirent {
* char d_name[ NAME_MAX + 1 ];
* other stuff;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other way around, I believe. d_name must be the last field.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@splinterofchaos Indeed, thank you for catching this!

Tested on Linux, MacOS X, SmartOS and Windows.

Fixes joyent#1430.
@misterdjules misterdjules changed the title fs: uv_fs_{open,read}_dir for unix fs: uv_fs_{open,read,close}_dir Dec 3, 2014
@saghul
Copy link
Contributor

saghul commented Dec 16, 2014

Sorry, this fell through the cracks, can you please reopen it at libuv/libuv? Thanks!

@saghul saghul closed this Dec 16, 2014
misterdjules pushed a commit to misterdjules/libuv-1 that referenced this pull request Jan 30, 2015
This is the same changes as joyent/libuv#1574.
This commit is just the start of getting them to work in libuv/libuv.

Failing tests will be fixed asap.

Fixes libuv#170.
whitlockjc pushed a commit to whitlockjc/libuv that referenced this pull request Jun 30, 2015
This is the same changes as joyent/libuv#1574.
This commit is just the start of getting them to work in libuv/libuv.

Failing tests will be fixed asap.

Fixes libuv#170.
whitlockjc pushed a commit to whitlockjc/libuv that referenced this pull request Aug 19, 2015
This is the same changes as joyent/libuv#1574.
This commit is just the start of getting them to work in libuv/libuv.

Failing tests will be fixed asap.

Fixes libuv#170.
whitlockjc pushed a commit to whitlockjc/libuv that referenced this pull request Aug 10, 2016
This is the same changes as joyent/libuv#1574.
This commit is just the start of getting them to work in libuv/libuv.

Failing tests will be fixed asap.

Fixes libuv#170.
whitlockjc pushed a commit to whitlockjc/libuv that referenced this pull request Dec 22, 2016
This is the same changes as joyent/libuv#1574.
This commit is just the start of getting them to work in libuv/libuv.

Failing tests will be fixed asap.

Fixes libuv#170.
whitlockjc pushed a commit to whitlockjc/libuv that referenced this pull request Apr 14, 2018
This is the same changes as joyent/libuv#1574.
This commit is just the start of getting them to work in libuv/libuv.

Failing tests will be fixed asap.

Fixes libuv#170.
whitlockjc pushed a commit to whitlockjc/libuv that referenced this pull request Apr 14, 2018
This is the same changes as joyent/libuv#1574.
This commit is just the start of getting them to work in libuv/libuv.

Failing tests will be fixed asap.

Fixes libuv#170.
whitlockjc pushed a commit to whitlockjc/libuv that referenced this pull request Apr 15, 2018
This is the same changes as joyent/libuv#1574
but reworked for libuv/libuv.

This commit also updates `uv__fs_readdir` to use `readdir` instead of
the deprecated `readdir_r`.

Fixes libuv#170.
whitlockjc pushed a commit to whitlockjc/libuv that referenced this pull request Apr 16, 2018
This is the same changes as joyent/libuv#1574
but reworked for libuv/libuv.

This commit also updates `uv__fs_readdir` to use `readdir` instead of
the deprecated `readdir_r`.

Fixes libuv#170.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants