8000 src: minor cleanup for node_revert by jasnell · Pull Request #14864 · nodejs/node · GitHub
[go: up one dir, main page]

Skip to content

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to d 8000 escribe 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 :-)

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

@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.

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
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 <anna@addaleax.net>
@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
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 <anna@addaleax.net>
@MylesBorins MylesBorins mentioned this pull request Sep 10, 2017
MylesBorins pushed a commit that referenced this pull request Sep 12, 2017
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 <anna@addaleax.net>
@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

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

Development

Successfully merging this pull request may close these issues.

5 participants

0