win, fs: uv_fs_fchmod support for -A files by bzoz · Pull Request #1819 · libuv/libuv

bzoz · 2018-04-26T11:42:23Z

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 · 2018-04-26T11:44:08Z

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

bnoordhuis

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

bnoordhuis · 2018-04-27T11:43:15Z

+
+  /* 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 


Typo: read-only

bnoordhuis · 2018-04-27T11:44:19Z

+        SET_REQ_WIN32_ERROR(req, pRtlNtStatusToDosError(nt_status));
+        goto fchmod_cleanup;
+      }
+      /* Remeber to clear the flag later on */


Typo: remember

bnoordhuis · 2018-04-27T11:48:09Z

+      if (!NT_SUCCESS(nt_status)) {
+        SET_REQ_WIN32_ERROR(req, pRtlNtStatusToDosError(nt_status));
+        goto fchmod_cleanup;
+      }


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

bnoordhuis · 2018-04-27T11:49:48Z

+        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:


Typo: delete

bnoordhuis · 2018-04-27T11:50:27Z

+    r = call_icacls("icacls test_file_icacls /inheritance:r /remove \"%s\"",
+                    pwd.username);
+    if (r != 0) {
+        goto acl_cleanup;


Indent errors in this file too.

bzoz · 2018-04-27T13:19:14Z

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 · 2018-04-27T16:42:12Z

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

bnoordhuis · 2018-04-30T09:55:15Z

+  uv_fs_req_cleanup(&close_req);
+
+ acl_cleanup:
+  /* Cleanup */


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.

bnoordhuis · 2018-04-30T09:55:40Z

+}
+#endif
+
+#ifdef _WIN32


You can fold this into the previous #ifdef block.

bzoz · 2018-05-02T12:01:25Z

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/

Adds uv_fs_chmod support for files with the Archive attribute cleared Ref: libuv#1777 Ref: nodejs/node#12803 PR-URL: libuv#1819

bzoz · 2018-05-16T17:03:19Z

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/

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 <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

bzoz · 2018-05-17T07:27:48Z

Failures unrelated, landed in b59fc58

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 <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

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 <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

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 <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

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 <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

bnoordhuis reviewed Apr 27, 2018

View reviewed changes

bnoordhuis approved these changes Apr 30, 2018

View reviewed changes

bnoordhuis approved these changes May 2, 2018

View reviewed changes

cjihrig approved these changes May 3, 2018

View reviewed changes

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 force-pushed the bartek-archive-fchmod-take2 branch from a83ea33 to fcb058f Compare May 16, 2018 17:01

bzoz closed this May 17, 2018

bzoz mentioned this pull request May 17, 2018

test: regression test for Node issue #20112 #1818

Closed

Conversation

bzoz commented Apr 26, 2018

Uh oh!

bzoz commented Apr 26, 2018

Uh oh!

bnoordhuis left a comment

Choose a reason for hiding this comment

Uh oh!

bnoordhuis Apr 27, 2018

Choose a reason for hiding this comment

Uh oh!

bnoordhuis Apr 27, 2018

Choose a reason for hiding this comment

Uh oh!

bnoordhuis Apr 27, 2018

Choose a reason for hiding this comment

Uh oh!

bnoordhuis Apr 27, 2018

Choose a reason for hiding this comment

Uh oh!

bnoordhuis Apr 27, 2018

Choose a reason for hiding this comment

Uh oh!

bzoz commented Apr 27, 2018

Uh oh!

cjihrig commented Apr 27, 2018

Uh oh!

bnoordhuis Apr 30, 2018

Choose a reason for hiding this comment

Uh oh!

bnoordhuis Apr 30, 2018

Choose a reason for hiding this comment

Uh oh!

bzoz commented May 2, 2018

Uh oh!

bzoz commented May 16, 2018

Uh oh!

bzoz commented May 17, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants