Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

aix: don't EISDIR on read from directory fd #2025

Closed
wants to merge 1 commit into from
Closed

aix: don't EISDIR on read from directory fd #2025

wants to merge 1 commit into from

Conversation

bnoordhuis
Copy link
Member

Remove the artificial EISDIR that was generated when trying to
uv_fs_read() from a file descriptor that refers to a directory.

We don't do that on the BSDs either (where reading from a directory
is allowed) and it introduces an extra stat() call for every read.

Refs: #2023 (comment)

@bnoordhuis
aix: don't EISDIR on read from directory fd
389c277 
Remove the artificial EISDIR that was generated when trying to
uv_fs_read() from a file descriptor that refers to a directory.

We don't do that on the BSDs either (where reading from a directory
is allowed) and it introduces an extra stat() call for every read.

Refs: libuv#2023 (comment)
@bnoordhuis bnoordhuis mentioned this pull request Oct 8, 2018
Closed
@bnoordhuis
Copy link
Member Author

CI: https://ci.nodejs.org/view/libuv/job/libuv-test-commit/1052/

I couldn't find tests that check for the presence/absence of EISDIR. If the CI run turns up green, I'll see about adding some.

bnoordhuis added a commit to bnoordhuis/io.js that referenced this pull request Oct 8, 2018
@bnoordhuis
test: read() on dir on AIX does not return EISDIR
75d6134 
An upcoming change in libuv will remove the artificial EISDIR error.
Update the test to reflect that.

Refs: libuv/libuv#2025
@bnoordhuis bnoordhuis mentioned this pull request Oct 8, 2018
Closed
@bnoordhuis
Copy link
Member Author

cc @libuv/aix

@thefourtheye
Copy link
Contributor

I couldn't find tests that check for the presence/absence of EISDIR. If the CI run turns up green, I'll see about adding some.

I tried to implement a test for the same in #2023. Perhaps we could use that?

cjihrig
cjihrig approved these changes Oct 8, 2018
View reviewed changes
Copy link
Contributor

@cjihrig cjihrig left a comment •
edited
Loading

Choose a reason for hiding this comment

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

libuv CI was good. libuv+Node CI: https://ci.nodejs.org/view/libuv/job/libuv-in-node/66/

EDIT: AIX failure in libuv+Node CI, but it is expected (nodejs/node#23330)

cjihrig pushed a commit that referenced this pull request Oct 8, 2018
@bnoordhuis @cjihrig
aix: don't EISDIR on read from directory fd
25a3894 
Remove the artificial EISDIR that was generated when trying to
uv_fs_read() from a file descriptor that refers to a directory.

We don't do that on the BSDs either (where reading from a directory
is allowed) and it introduces an extra stat() call for every read.

Refs: #2023 (comment)
PR-URL: #2025
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
@cjihrig
Copy link
Contributor

cjihrig commented Oct 8, 2018

Landed in 25a3894. Thanks Ben!

@cjihrig cjihrig closed this Oct 8, 2018
@santigimeno santigimeno mentioned this pull request Oct 8, 2018
Closed
Trott pushed a commit to Trott/io.js that referenced this pull request Oct 8, 2018
@bnoordhuis @Trott
test: read() on dir on AIX does not return EISDIR
787cbe9 
An upcoming change in libuv will remove the artificial EISDIR error.
Update the test to reflect that.

Refs: libuv/libuv#2025

PR-URL: nodejs#23330
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@vtjnash
Copy link
Member

vtjnash commented Oct 9, 2018

Thanks!

targos pushed a commit to nodejs/node that referenced this pull request Oct 10, 2018
@bnoordhuis @targos
test: read() on dir on AIX does not return EISDIR
b2d3445 
An upcoming change in libuv will remove the artificial EISDIR error.
Update the test to reflect that.

Refs: libuv/libuv#2025

PR-URL: #23330
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
jasnell pushed a commit to nodejs/node that referenced this pull request Oct 17, 2018
@bnoordhuis @jasnell
test: read() on dir on AIX does not return EISDIR
b48b1e5 
An upcoming change in libuv will remove the artificial EISDIR error.
Update the test to reflect that.

Refs: libuv/libuv#2025

PR-URL: #23330
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Nov 5, 2018
@bnoordhuis @MylesBorins
test: read() on dir on AIX does not return EISDIR
3a5374c 
An upcoming change in libuv will remove the artificial EISDIR error.
Update the test to reflect that.

Refs: libuv/libuv#2025

PR-URL: nodejs#23330
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
MylesBorins pushed a commit to nodejs/node that referenced this pull request Nov 11, 2018
@bnoordhuis @MylesBorins
test: read() on dir on AIX does not return EISDIR
dbc7d9b 
An upcoming change in libuv will remove the artificial EISDIR error.
Update the test to reflect that.

Refs: libuv/libuv#2025

Backport-PR-URL: #24103
PR-URL: #23330
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@kadler kadler mentioned this pull request Jan 10, 2019
Closed
@sam-github
Copy link
Contributor

nodejs/node#25433

@kadler kadler mentioned this pull request Jan 15, 2019
Closed
@richardlau richardlau mentioned this pull request Jun 23, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thefourtheye thefourtheye thefourtheye approved these changes

@santigimeno santigimeno santigimeno approved these changes

@cjihrig cjihrig cjihrig approved these changes

@richardlau richardlau richardlau approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@bnoordhuis @thefourtheye @cjihrig @vtjnash @sam-github @santigimeno @richardlau