first go at fixing #1790 (lchown part) #1826

simevo · 2018-04-27T20:01:15Z

TODO:

move platform-specfic code to the right place
test
doc

cjihrig

This is missing Windows pieces, tests, and docs.

cjihrig · 2018-04-28T23:23:40Z

include/uv.h

                           uv_uid_t uid,
                           uv_gid_t gid,
                           uv_fs_cb cb);
+UV_EXTERN int uv_ls_chown(uv_loop_t* loop,


uv_fs_lchown()?

cjihrig · 2018-04-28T23:24:22Z

src/unix/fs.c


+int uv_fs_lchown(uv_loop_t* loop,
+                 uv_fs_t* req,
+                 uv_os_fd_t file,


The signature doesn't match up.

cjihrig · 2018-04-28T23:25:51Z

src/unix/fs.c

+                 uv_gid_t gid,
+                 uv_fs_cb cb) {
+  INIT(LCHOWN);
+  req->file = file;


This doesn't seem correct to me.

simevo · 2018-04-29T07:27:45Z

the existing chown and fchown functions "are not implemented on Windows":
http://docs.libuv.org/en/v1.x/fs.html#c.uv_fs_chown
so neither should be these ones I believe.

there's a TODO in the tests because I have not though yet about how to check that lchown really does not change the linked-to file

also:

not ok 45 - fs_chown
# exit code 6
# Output from process `fs_chown`:
# Assertion failed in ../test/test-fs.c on line 1561: r == 0

cjihrig · 2018-04-29T10:11:23Z

the existing chown and fchown functions "are not implemented on Windows":

There are still stubs in the code -

libuv/src/win/fs.c

Line 2258 in ff167ea

int uv_fs_fchown(uv_loop_t* loop, uv_fs_t* req, uv_file fd, uv_uid_t uid,

.

simevo · 2018-04-30T06:56:26Z

Now it seems to compile and generate the library fine on windows with VS 2015.

There's still the test failing and the missing test (see TODO in test/test-fs.c).
I wonder what user id -1 stands for, on my system:

grep UID /etc/login.defs 
UID_MIN                  1000
UID_MAX                 60000
...

simevo · 2018-05-22T10:19:54Z

Hi what else can I do to get this PR into libuv ? Thanks

cjihrig · 2018-05-22T20:03:12Z

include/uv.h

                           uv_gid_t gid,
                           uv_fs_cb cb);
+UV_EXTERN int uv_fs_lchown(uv_loop_t* loop,
+                          uv_fs_t* req,


Can you line all of these arguments up with the uv_loop_t on the previous line.

cjihrig · 2018-05-22T20:05:14Z

src/unix/fs.c



+int uv_fs_lchown(uv_loop_t* loop,
+                uv_fs_t* req,


Please line the arguments up.

cjihrig · 2018-05-22T20:13:25Z

test/test-fs.c

+static void lchown_cb(uv_fs_t* req) {
+  ASSERT(req->fs_type == UV_FS_LCHOWN);
+  ASSERT(req->result == 0);
+  chown_cb_count++;


This should be lchown_cb_count?

cjihrig · 2018-05-22T20:15:14Z

test/test-fs.c

 static int fchmod_cb_count;
 static int chown_cb_count;
 static int fchown_cb_count;
+static int lchown_cb_count;


This variable isn't asserted on anywhere.

cjihrig · 2018-05-22T20:15:24Z

test/test-fs.c

  uv_fs_req_cleanup(req);
 }

+static void lchown_cb(uv_fs_t* req) {


This isn't called.

cjihrig · 2018-05-22T20:16:45Z

@bnoordhuis do you have the historical context on why these functions just return 0 on Windows instead of ENOSYS?

bnoordhuis · 2018-05-23T11:08:51Z

Not sure what you mean. What does "these functions" refer to?

cjihrig · 2018-05-23T12:01:41Z

Sorry, "these functions" refers to the chown family of functions.

bnoordhuis · 2018-05-23T16:54:00Z

It's because there is no meaningful way to implement them, user/group/other isn't a thing on Windows.

I don't think it was a deliberate decision to quietly ignore instead of returning ENOSYS, just something that was deemed Good Enough at the time and made node's test suite pass. :-)

cjihrig · 2018-05-23T17:04:29Z

I don't think it was a deliberate decision to quietly ignore instead of returning ENOSYS, just something that was deemed Good Enough at the time and made node's test suite pass.

That's what I was looking for. Thanks!

- increment lchown_cb_count counter - also test async lchown and thereby assert on lchown_cb_count and call lchown_cb - use gid = -1, as per [1] the corresponding ID of the file will not be changed - on cleanup, also remove test_file_link [1] http://pubs.opengroup.org/onlinepubs/9699919799//functions/chown.html

cjihrig · 2018-05-26T02:57:56Z

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

simevo · 2018-05-26T05:07:44Z

all CI failures seem unrelated to me:

https://ci.nodejs.org/job/libuv-test-commit-linux/910/nodes=debian7-docker-armv7/

ERROR: Error cloning remote repo 'origin'

https://ci.nodejs.org/job/libuv-test-commit-linux/910/nodes=alpine-last-latest-x64/
https://ci.nodejs.org/job/libuv-test-commit-linux/910/nodes=alpine-latest-x64/

not ok 20 - connect_unspecified
# exit code 134
# Output from process `connect_unspecified`:
# Assertion failed in test/test-connect-unspecified.c on line 56: uv_tcp_connect(&connect6, &socket6, (const struct sockaddr*) &addr6, connect_6) == 0

https://ci.nodejs.org/job/libuv-test-commit-linux/910/nodes=centos6-64/
https://ci.nodejs.org/job/libuv-test-commit-linux/910/nodes=centos6-64-gcc44/
https://ci.nodejs.org/job/libuv-test-commit-linux/910/nodes=ubuntu1404-gyp-64/

not ok 103 - getaddrinfo_fail_sync
# timeout
# Output from process `getaddrinfo_fail_sync`: (no output)

https://ci.nodejs.org/job/libuv-test-commit-aix/879/nodes=aix61-ppc64/

not ok 250 - tcp_ping_pong
# exit code 393222
# Output from process `tcp_ping_pong`: (no output)
Output from process `tcp_ping_pong`:
# Assertion failed in test/test-ping-pong.c on line 144: status == 0
...
not ok 256 - tcp_ref3
# exit code 393222
# Output from process `tcp_ref3`: (no output)
Output from process `tcp_ref3`:
# Assertion failed in test/test-ref.c on line 96: status == 0
...
not ok 262 - tcp_write_fail
# exit code 393222
# Output from process `tcp_write_fail`: (no output)
Output from process `tcp_write_fail`:
# Assertion failed in test/test-tcp-write-fail.c on line 77: status == 0

https://ci.nodejs.org/job/libuv-test-commit-zos/724/nodes=zos-s390x/

not ok 309 - udp_multicast_join6
# exit code 3
# Output from process `udp_multicast_join6`:
# Assertion failed in ../test/test-udp-multicast-join6.c on line 136: r == 0
# CEE5207E The signal SIGABRT was received.

cjihrig

I'm not sure if targeting master vs. v1.x is necessary, but LGTM.

Fixes: #1790 PR-URL: #1826 Reviewed-By: Colin Ihrig <[email protected]>

cjihrig · 2018-06-19T13:50:17Z

Landed on master in 33b6db3. Thanks!

I'm going to look into backporting this to v1.x as well.

Fixes: libuv#1790 PR-URL: libuv#1826 Reviewed-By: Colin Ihrig <[email protected]>

cjihrig · 2018-06-19T14:20:08Z

Landed on v1.x in aa28f7d.

Here is a CI run of that commit with no related failures: https://ci.nodejs.org/view/libuv/job/libuv-test-commit/940/

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]>

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]>

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]>

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]>

first go at fixing libuv#1790 (lchown part)

3da7f02

cjihrig requested changes Apr 28, 2018

View reviewed changes

Paolo Greppi added 3 commits April 29, 2018 09:18

fix header, signature and implementation

e172793

doc

ae7f254

WIP testing lchown

01216a9

Paolo Greppi added 2 commits April 30, 2018 08:46

implement windows stubs

c6de166

fixed err undeclared id

85dcd23

cjihrig requested changes May 22, 2018

View reviewed changes

Paolo Greppi added 3 commits May 24, 2018 11:11

line up arguments to uv_fs_lchown

ef50845

doc

3d06603

cjihrig approved these changes May 26, 2018

View reviewed changes

cjihrig pushed a commit that referenced this pull request Jun 19, 2018

unix,win: add uv_fs_lchown()

33b6db3

Fixes: #1790 PR-URL: #1826 Reviewed-By: Colin Ihrig <[email protected]>

cjihrig closed this Jun 19, 2018

cjihrig mentioned this pull request Jun 19, 2018

uv_fs_futime implementation for linux #1790

Closed

cjihrig pushed a commit to cjihrig/libuv that referenced this pull request Jun 19, 2018

unix,win: add uv_fs_lchown()

aa28f7d

Fixes: libuv#1790 PR-URL: libuv#1826 Reviewed-By: Colin Ihrig <[email protected]>

simevo mentioned this pull request Jun 19, 2018

fs.lchown should be undeprecated nodejs/node#19868

Closed

fsateler mentioned this pull request Jul 4, 2018

#1826 broke ABI compat without SONAME bump #1908

Closed

first go at fixing #1790 (lchown part) #1826

first go at fixing #1790 (lchown part) #1826

Uh oh!

Conversation

simevo commented Apr 27, 2018

Uh oh!

cjihrig left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

simevo commented Apr 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cjihrig commented Apr 29, 2018

Uh oh!

simevo commented Apr 30, 2018

Uh oh!

simevo commented May 22, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjihrig commented May 22, 2018

Uh oh!

bnoordhuis commented May 23, 2018

Uh oh!

cjihrig commented May 23, 2018

Uh oh!

bnoordhuis commented May 23, 2018

Uh oh!

cjihrig commented May 23, 2018

Uh oh!

cjihrig commented May 26, 2018

Uh oh!

simevo commented May 26, 2018

Uh oh!

cjihrig left a comment

Choose a reason for hiding this comment

Uh oh!

cjihrig commented Jun 19, 2018

Uh oh!

cjihrig commented Jun 19, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

simevo commented Apr 29, 2018 •

edited

Loading