Skip to content

win, fs: uv_fs_fchmod support for -A files #1819

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
Closed
bzoz wants to merge 1 commit into from
Closed

win, fs: uv_fs_fchmod support for -A files #1819

bzoz wants to merge 1 commit into from

Conversation

@bzoz
Copy link
Member

@bzoz bzoz commented Apr 26, 2018

Adds uv_fs_chmod support for files with the Archive attribute cleared.

This PR assumes that #1818 landed, for the regression test.

Ref: #1777
Ref: nodejs/node#12803

@bzoz
Copy link
Member Author

bzoz commented Apr 26, 2018

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

bnoordhuis
bnoordhuis reviewed Apr 27, 2018
View reviewed changes
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

Wouldn't it make sense to coalesce the first NtSetInformationFile() call with the one on line 1544?

src/win/fs.c

/* Test if the Archive attribute is cleared */
if ((file_info.FileAttributes & FILE_ATTRIBUTE_ARCHIVE) == 0) {
/* Set Archive flag, otherwise setting or clearing the read-olny
Copy link
Member

@bnoordhuis bnoordhuis Apr 27, 2018

Choose a reason for hiding this comment

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

Typo: read-only

src/win/fs.c
SET_REQ_WIN32_ERROR(req, pRtlNtStatusToDosError(nt_status));
goto fchmod_cleanup;
}
/* Remeber to clear the flag later on */
Copy link
Member

@bnoordhuis bnoordhuis Apr 27, 2018

Choose a reason for hiding this comment

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

Typo: remember

src/win/fs.c
if (!NT_SUCCESS(nt_status)) {
SET_REQ_WIN32_ERROR(req, pRtlNtStatusToDosError(nt_status));
goto fchmod_cleanup;
}
Copy link
Member

@bnoordhuis bnoordhuis Apr 27, 2018

Choose a reason for hiding this comment

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

Can you fix the indentation? It should be two spaces, not four.

test/test-fs.c
Based on Node.js test from
https://github.com/nodejs/node/commit/3ba81e34e86a5c32658e218cb6e65b13e8326bc5
If anything goes wrong, you can delte the test_fle_icacls with:
Copy link
Member

@bnoordhuis bnoordhuis Apr 27, 2018

Choose a reason for hiding this comment

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

Typo: delete

test/test-fs.c
r = call_icacls("icacls test_file_icacls /inheritance:r /remove \"%s\"",
pwd.username);
if (r != 0) {
goto acl_cleanup;
Copy link
Member

@bnoordhuis bnoordhuis Apr 27, 2018

Choose a reason for hiding this comment

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

Indent errors in this file too.

@bzoz
Copy link
Member Author

bzoz commented Apr 27, 2018

Updated, PTAL.
New CI: https://ci.nodejs.org/view/libuv/job/libuv-test-commit/816/

When developing this it looked like NtSetInformatonFile did not work at all when Archive flag was cleared, thus this Archive shenanigans. But it turns out that FileAttributes cannot be 0 - for all flags cleared it has to be set to FILE_ATTRIBUTE_NORMAL and everything works like intended.

@cjihrig
Copy link
Contributor

cjihrig commented Apr 27, 2018

Can you also run the libuv-in-node CI job on this PR prior to landing.

bnoordhuis
bnoordhuis approved these changes Apr 30, 2018
View reviewed changes
test/test-fs.c Outdated
uv_fs_req_cleanup(&close_req);

acl_cleanup:
/* Cleanup */
Copy link
Member

@bnoordhuis bnoordhuis Apr 30, 2018

Choose a reason for hiding this comment

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

Superfluous comment. As a general observation, most of the comments just say what the code does, not why; not that useful. I'd remove them.

test/test-fs.c
}
#endif

#ifdef _WIN32
Copy link
Member

@bnoordhuis bnoordhuis Apr 30, 2018

Choose a reason for hiding this comment

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

You can fold this into the previous #ifdef block.

@bzoz
Copy link
Member Author

bzoz commented May 2, 2018

Updated, PTAL.

CI: https://ci.nodejs.org/view/libuv/job/libuv-test-commit/822/
Node CI: https://ci.nodejs.org/view/libuv/job/libuv-in-node/16/

@bzoz
win, fs: uv_fs_fchmod support for -A files
fcb058f 
Adds uv_fs_chmod support for files with the Archive attribute cleared

Ref: libuv#1777
Ref: nodejs/node#12803
PR-URL: libuv#1819
@bzoz bzoz force-pushed the bartek-archive-fchmod-take2 branch from a83ea33 to fcb058f Compare May 16, 2018 17:01
@bzoz
Copy link
Member Author

bzoz commented May 16, 2018

Old CI 404, rebased, new CI

CI node: https://ci.nodejs.org/view/libuv/job/libuv-in-node/18/
CI: https://ci.nodejs.org/view/libuv/job/libuv-test-commit/841/

bzoz added a commit that referenced this pull request May 17, 2018
@bzoz
win, fs: uv_fs_fchmod support for -A files
b59fc58 
Adds uv_fs_chmod support for files with the Archive attribute cleared

Ref: #1777
Ref: nodejs/node#12803
PR-URL: #1819
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@bzoz
Copy link
Member Author

bzoz commented May 17, 2018

Failures unrelated, landed in b59fc58

@bzoz bzoz closed this May 17, 2018
@bzoz bzoz mentioned this pull request May 17, 2018
Closed
cjihrig added a commit to cjihrig/node that referenced this pull request Jun 25, 2018
@cjihrig
deps: upgrade to libuv 1.21.0
537a4ba 
Notable changes:

- Building via cmake is now supported.
  PR-URL: libuv/libuv#1850
- Stricter checks have been added to prevent watching the same
  file descriptor multiple times.
  PR-URL: libuv/libuv#1851
  Refs: nodejs#3604
- An IPC deadlock on Windows has been fixed.
  PR-URL: libuv/libuv#1843
  Fixes: nodejs#9706
  Fixes: nodejs#7657
- uv_fs_lchown() has been added.
  PR-URL: libuv/libuv#1826
  Refs: nodejs#19868
- uv_fs_copyfile() sets errno on error.
  PR-URL: libuv/libuv#1881
  Fixes: nodejs#21329
- uv_fs_fchmod() supports -A files on Windows.
  PR-URL: libuv/libuv#1819
  Refs: nodejs#12803

PR-URL: nodejs#21466
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit to nodejs/node that referenced this pull request Jun 25, 2018
@cjihrig @targos
deps: upgrade to libuv 1.21.0
f162939 
Notable changes:

- Building via cmake is now supported.
  PR-URL: libuv/libuv#1850
- Stricter checks have been added to prevent watching the same
  file descriptor multiple times.
  PR-URL: libuv/libuv#1851
  Refs: #3604
- An IPC deadlock on Windows has been fixed.
  PR-URL: libuv/libuv#1843
  Fixes: #9706
  Fixes: #7657
- uv_fs_lchown() has been added.
  PR-URL: libuv/libuv#1826
  Refs: #19868
- uv_fs_copyfile() sets errno on error.
  PR-URL: libuv/libuv#1881
  Fixes: #21329
- uv_fs_fchmod() supports -A files on Windows.
  PR-URL: libuv/libuv#1819
  Refs: #12803

PR-URL: #21466
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Nov 5, 2018
@cjihrig @MylesBorins
deps: upgrade to libuv 1.21.0
4de5458 
Notable changes:

- Building via cmake is now supported.
  PR-URL: libuv/libuv#1850
- Stricter checks have been added to prevent watching the same
  file descriptor multiple times.
  PR-URL: libuv/libuv#1851
  Refs: nodejs#3604
- An IPC deadlock on Windows has been fixed.
  PR-URL: libuv/libuv#1843
  Fixes: nodejs#9706
  Fixes: nodejs#7657
- uv_fs_lchown() has been added.
  PR-URL: libuv/libuv#1826
  Refs: nodejs#19868
- uv_fs_copyfile() sets errno on error.
  PR-URL: libuv/libuv#1881
  Fixes: nodejs#21329
- uv_fs_fchmod() supports -A files on Windows.
  PR-URL: libuv/libuv#1819
  Refs: nodejs#12803

PR-URL: nodejs#21466
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit to nodejs/node that referenced this pull request Nov 11, 2018
@cjihrig @MylesBorins
deps: upgrade to libuv 1.21.0
11cb09b 
Notable changes:

- Building via cmake is now supported.
  PR-URL: libuv/libuv#1850
- Stricter checks have been added to prevent watching the same
  file descriptor multiple times.
  PR-URL: libuv/libuv#1851
  Refs: #3604
- An IPC deadlock on Windows has been fixed.
  PR-URL: libuv/libuv#1843
  Fixes: #9706
  Fixes: #7657
- uv_fs_lchown() has been added.
  PR-URL: libuv/libuv#1826
  Refs: #19868
- uv_fs_copyfile() sets errno on error.
  PR-URL: libuv/libuv#1881
  Fixes: #21329
- uv_fs_fchmod() supports -A files on Windows.
  PR-URL: libuv/libuv#1819
  Refs: #12803

Backport-PR-URL: #24103
PR-URL: #21466
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@cjihrig cjihrig cjihrig 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.

3 participants

@bzoz @cjihrig @bnoordhuis