Skip to content
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

src: minor cleanup for node_revert #14864

Closed
wants to merge 1 commit into from
Closed

src: minor cleanup for node_revert #14864

wants to merge 1 commit into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Aug 16, 2017

Make the revert related functions inline to eliminate the need for node_revert.cc, prefix the constants and the def, other misc cleanup

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 build Issues and PRs related to build files or the CI. c++ Issues and PRs that require attention from people who are familiar with C++. labels Aug 16, 2017
addaleax
addaleax reviewed Aug 16, 2017
View reviewed changes
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

LGTM pending reverted being a single global variable again

src/node_revert.h Outdated

static unsigned int reverted = 0;
Copy link
Member

Choose a reason for hiding this comment

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

This can’t be static, it would give every source file an independent copy.

Copy link
Member Author

Choose a reason for hiding this comment

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

doh... sigh.

Copy link
Member Author

Choose a reason for hiding this comment

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

fwiw, node_revert.h is only imported by node.cc, but point taken :-)

Copy link
Member

Choose a reason for hiding this comment

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

That’s partly because we’re not actually reverting anything currently. ;)

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed :-)

addaleax
addaleax approved these changes Aug 16, 2017
View reviewed changes
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@refack
Copy link
Contributor

refack commented Aug 16, 2017

Quastion: what is it used for?

@jasnell
Copy link
Member Author

jasnell commented Aug 17, 2017

On the hopefully quite rare occasion when a critical security fix is made, there is a non-zero possibility that it could break someone's code. The --security-revert mechanism provides users with a mechanism for selectively switching off selected security fixes. This has only been used once before in Node.js 4.x (https://github.com/nodejs/node/blob/v4.x/src/node_revert.h#L16).

This is one of those odd bits of code that ideally would not be used but is important to have just in case.

@jasnell
Copy link
Member Author

jasnell commented Aug 17, 2017

CI: https://ci.nodejs.org/job/node-test-pull-request/9716/

@jasnell
Copy link
Member Author

jasnell commented Aug 17, 2017

Cancelled the CI... weird failures there... trying again: https://ci.nodejs.org/job/node-test-pull-request/9717/

@jasnell
Copy link
Member Author

jasnell commented Aug 17, 2017

Ah... after rebasing there was a reverted commit that was messing things up... trying again: CI: https://ci.nodejs.org/job/node-test-pull-request/9718/

@jasnell
Copy link
Member Author

jasnell commented Aug 17, 2017

CI is good. Failures there are unrelated.

@jasnell
src: minor cleanup for node_revert
714c716 
Make the revert related functions inline to eliminate the need
for node_revert.cc, prefix the constants and the def, other misc
cleanup
jasnell added a commit that referenced this pull request Aug 18, 2017
@jasnell
src: minor cleanup for node_revert
35f6e59 
Make the revert related functions inline to eliminate the need
for node_revert.cc, prefix the constants and the def, other misc
cleanup

PR-URL: #14864
Reviewed-By: Anna Henningsen <[email protected]>
@jasnell
Copy link
Member Author

jasnell commented Aug 18, 2017

Landed in 35f6e59

@jasnell jasnell closed this Aug 18, 2017
MylesBorins pushed a commit that referenced this pull request Sep 10, 2017
@jasnell @MylesBorins
src: minor cleanup for node_revert
6326d20 
Make the revert related functions inline to eliminate the need
for node_revert.cc, prefix the constants and the def, other misc
cleanup

PR-URL: #14864
Reviewed-By: Anna Henningsen <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Sep 10, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Sep 12, 2017
@jasnell @MylesBorins
src: minor cleanup for node_revert
081c3e1 
Make the revert related functions inline to eliminate the need
for node_revert.cc, prefix the constants and the def, other misc
cleanup

PR-URL: #14864
Reviewed-By: Anna Henningsen <[email protected]>
@MylesBorins
Copy link
Contributor

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if no let me know or add the dont-land-on label.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@addaleax addaleax addaleax approved these changes

Assignees
No one assigned
Labels
build Issues and PRs related to build files or the CI. c++ Issues and PRs that require attention from people who are familiar with C++.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jasnell @refack @MylesBorins @addaleax @nodejs-github-bot