Skip to content

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Nov 3, 2017

This commit adds uv_fs_req_cleanup() calls to all uses of uv_fs_t's in src/module_wrap.cc.

Note, this originally started as a response to a Coverity scan:

>>>     CID 178567:  Error handling issues  (CHECKED_RETURN)
>>>     Calling "uv_fs_read" without checking return value (as is done elsewhere 4 out of 5 times).
314         uv_fs_read(uv_default_loop(),

I fixed the Coverity issue by using the return value from uv_fs_read() instead of req.result.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

src

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Nov 3, 2017
@addaleax addaleax added the esm Issues and PRs related to the ECMAScript Modules implementation. label Nov 3, 2017
@cjihrig
Copy link
Contributor Author

cjihrig commented Nov 6, 2017

This commit adds uv_fs_req_cleanup() calls to all uses of
uv_fs_t's in src/module_wrap.cc.

PR-URL: nodejs#16722
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@cjihrig cjihrig merged commit 4023702 into nodejs:master Nov 6, 2017
@cjihrig cjihrig deleted the cleanup branch November 6, 2017 15:36
cjihrig added a commit to cjihrig/node that referenced this pull request Nov 6, 2017
This commit adds uv_fs_req_cleanup() calls to all uses of
uv_fs_t's in src/module_wrap.cc.

PR-URL: nodejs#16722
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@cjihrig cjihrig mentioned this pull request Nov 6, 2017
gibfahn pushed a commit that referenced this pull request Dec 19, 2017
This commit adds uv_fs_req_cleanup() calls to all uses of
uv_fs_t's in src/module_wrap.cc.

PR-URL: #16722
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@gibfahn
Copy link
Member

gibfahn commented Dec 19, 2017

This seems uncontroversial, so I've backported it to v8.x. LMK if there's a reason not to.

This was referenced Dec 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. esm Issues and PRs related to the ECMAScript Modules implementation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants