Skip to content

Conversation

@cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Jun 14, 2018

When the specific value of -1 is returned, uv__fs_work() uses the value of errno. This commit sets errno in uv__fs_copyfile().

Fixes: nodejs/node#21329

/* If -1 is returned, errno is used. */
if (result != 0)
errno = result;

Copy link
Member

Choose a reason for hiding this comment

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

Not wrong, not exactly, but should probably be along these lines:

if (result == 0)
  return 0;

errno = result;
return -1;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated. I had to add errno = UV__ERR(result);. Otherwise there were some issues regarding the sign on errno because UV__ERR() is called again in uv__fs_work().

Copy link
Member

Choose a reason for hiding this comment

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

Right you are.

src/unix/fs.c Outdated
}
}

/* If -1 is returned, errno is used. */
Copy link
Member

Choose a reason for hiding this comment

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

You could point out that this can happen on some systems with UV_EPERM.

@cjihrig cjihrig force-pushed the copyfile branch 2 times, most recently from a96d721 to fdce70c Compare June 14, 2018 16:01
@cjihrig
Copy link
Contributor Author

cjihrig commented Jun 14, 2018

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

The one Windows failure is unrelated. macOS is having some infrastructure problems, but passes fine for me locally.

When the specific value of -1 is returned, uv__fs_work() uses
the value of errno. This commit sets errno in uv__fs_copyfile().

Fixes: nodejs/node#21329
PR-URL: libuv#1881
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
cjihrig added a commit that referenced this pull request Jun 14, 2018
When the specific value of -1 is returned, uv__fs_work() uses
the value of errno. This commit sets errno in uv__fs_copyfile().

Fixes: nodejs/node#21329
PR-URL: #1881
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
@cjihrig
Copy link
Contributor Author

cjihrig commented Jun 14, 2018

Landed in e179bd1.

@cjihrig cjihrig closed this Jun 14, 2018
@cjihrig cjihrig deleted the copyfile branch June 14, 2018 16:52
cjihrig added a commit to cjihrig/node that referenced this pull request Jun 25, 2018
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
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
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
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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants