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.

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

src: create a getter for kernel version #31732

Closed

Conversation

juanarbol
Copy link
Member

@juanarbol juanarbol commented Feb 11, 2020

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. os Issues and PRs related to the os subsystem. labels Feb 11, 2020
lib/os.js Outdated Show resolved Hide resolved
lib/os.js Outdated Show resolved Hide resolved
lib/os.js Outdated
getOSType: _getOSType,
getPriority: _getPriority,
getTotalMem,
getUserInfo,
getUptime,
isBigEndian,
getOSVersion,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
getOSVersion,

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the review sr!

@juanarbol
Copy link
Member Author

I've just made a refactor with the internals implementation, we were using uv_os_uname. two times, then I've propose a third time, so I've just create an abstraction for this three calls into one.

src/node_os.cc Outdated
"release",
NewStringType::kNormal).ToLocalChecked(),
release_str)
.FromJust();
Copy link
Member

@addaleax addaleax Feb 13, 2020

Choose a reason for hiding this comment

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

It might be nicer to create an Array with 3 entries here, even if that means that means that the order of the entries becomes important:

  1. It removes the need to create strings for the keys in C++, and
  2. The .FromJust() calls here can be brought to crash if userland code runs before the os module is required

E.g. the following would current crash the process:

$ node -e 'Object.defineProperty(Object.prototype, "release", { set() { throw new Error(); } }); require("os")'
FATAL ERROR: v8::FromJust Maybe value is Nothing.
 1: 0xa15eb0 node::Abort() [./node]

Copy link
Member

Choose a reason for hiding this comment

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

Fwiw, here’s a good example of how to create a JS Array from C++ without much overhead:

node/src/node_util.cc

Lines 100 to 106 in d63bcdd

Local<Value> ret[] = {
proxy->GetTarget(),
proxy->GetHandler()
};
args.GetReturnValue().Set(
Array::New(args.GetIsolate(), ret, arraysize(ret)));

Copy link
Member

@santigimeno santigimeno Feb 13, 2020

Choose a reason for hiding this comment

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

E.g. the following would current crash the process:

Are these kind of crashes something to be avoided? It can be reproduced with some other properties:

node -e 'Object.defineProperty(Object.prototype, "name", { set() { throw new Error(); } });require("dns").resolveSrv("_jabber._tcp.google.com", () => {});'
FATAL ERROR: v8::FromJust Maybe value is Nothing.
 1: 0xa9d570 node::Abort() [node]

Copy link
Member

Choose a reason for hiding this comment

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

@santigimeno Yes – unfortunately, a lot of Node.js core C++ code doesn’t do error handling well, but new code generally should.

lib/os.js Show resolved Hide resolved
hola.js Outdated Show resolved Hide resolved
src/node_os.cc Outdated Show resolved Hide resolved
src/node_os.cc Outdated Show resolved Hide resolved
src/node_os.cc Outdated Show resolved Hide resolved
@juanarbol
Copy link
Member Author

Is this a semver-minor thing?

@addaleax addaleax added the semver-minor PRs that contain new features and should be released in the next minor version. label Mar 4, 2020
@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 4, 2020
@nodejs-github-bot
Copy link
Collaborator

addaleax pushed a commit that referenced this pull request Mar 9, 2020
PR-URL: #31732
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@addaleax
Copy link
Member

addaleax commented Mar 9, 2020

Landed in c1e6725 🎉

@addaleax addaleax closed this Mar 9, 2020
@cjihrig cjihrig mentioned this pull request Mar 9, 2020
3 tasks
@MylesBorins
Copy link
Contributor

This does not land cleanly on v13.x. Would someone be willing to open a backport?

@juanarbol
Copy link
Member Author

MEEEEEEE ME ME ME ME ME MEEEEEEE, I want to

@MylesBorins
Copy link
Contributor

Amazing, thanks @juanarbol

You can find details on how to do a backport in this guide

You wil lwant to target the v13.x-staging branch

MylesBorins pushed a commit that referenced this pull request Mar 10, 2020
Backport-PR-URL: #32166
PR-URL: #31732
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Mar 10, 2020
MylesBorins added a commit that referenced this pull request Mar 10, 2020
Notable changes:

* [[`ff58854dbe`](ff58854dbe)] - **(SEMVER-MINOR)** **fs**: return first folder made by mkdir recursive (Benjamin Coe) [#31530](#31530)
* [[`258a80d3cc`](258a80d3cc)] - **(SEMVER-MINOR)** **src**: create a getter for kernel version (Juan José Arboleda) [#31732](#31732)
* [[`4d5981be96`](4d5981be96)] - **(SEMVER-MINOR)** **async_hooks**: add sync enterWith to ALS (Stephen Belanger) [#31945](#31945)
* [[`dd83bd266d`](dd83bd266d)] - **(SEMVER-MINOR)** **wasi**: add returnOnExit option (Colin Ihrig) [#32101](#32101)

PR-URL: #32185
MylesBorins added a commit that referenced this pull request Mar 11, 2020
Notable changes:

* async_hooks:
  - add sync enterWith to ALS (Stephen Belanger)
    #31945
* cli:
  - allow --jitless V8 flag in NODE\_OPTIONS (Andrew Neitsch)
    #32100
* fs:
  - return first folder made by mkdir recursive (Benjamin Coe)
    #31530
* n-api:
  - define release 6 (Gabriel Schulhof)
    #32058
* src:
  - create a getter for kernel version (Juan José Arboleda)
    #31732
* wasi:
  - add returnOnExit option (Colin Ihrig)
    #32101

PR-URL: #32185
MylesBorins added a commit that referenced this pull request Mar 11, 2020
Notable changes:

* async_hooks:
  - add sync enterWith to ALS (Stephen Belanger)
    #31945
* cli:
  - allow --jitless V8 flag in NODE\_OPTIONS (Andrew Neitsch)
    #32100
* fs:
  - return first folder made by mkdir recursive (Benjamin Coe)
    #31530
* n-api:
  - define release 6 (Gabriel Schulhof)
    #32058
* src:
  - create a getter for kernel version (Juan José Arboleda)
    #31732
* wasi:
  - add returnOnExit option (Colin Ihrig)
    #32101

PR-URL: #32185
MylesBorins added a commit that referenced this pull request Mar 12, 2020
Notable changes:

* async_hooks:
  - add sync enterWith to ALS (Stephen Belanger)
    #31945
* cli:
  - allow --jitless V8 flag in NODE\_OPTIONS (Andrew Neitsch)
    #32100
* fs:
  - return first folder made by mkdir recursive (Benjamin Coe)
    #31530
* n-api:
  - define release 6 (Gabriel Schulhof)
    #32058
* src:
  - create a getter for kernel version (Juan José Arboleda)
    #31732
* wasi:
  - add returnOnExit option (Colin Ihrig)
    #32101

PR-URL: #32185
MylesBorins added a commit that referenced this pull request Mar 12, 2020
Notable changes:

* async_hooks:
  - add sync enterWith to ALS (Stephen Belanger)
    #31945
* cli:
  - allow --jitless V8 flag in NODE\_OPTIONS (Andrew Neitsch)
    #32100
* fs:
  - return first folder made by mkdir recursive (Benjamin Coe)
    #31530
* n-api:
  - define release 6 (Gabriel Schulhof)
    #32058
* src:
  - create a getter for kernel version (Juan José Arboleda)
    #31732
* wasi:
  - add returnOnExit option (Colin Ihrig)
    #32101

PR-URL: #32185
cjihrig added a commit to cjihrig/node that referenced this pull request Mar 13, 2020
pRtlGetVersion is not a thing. This text was likely a result of
copying the variable name used in libuv. This commit updates the
documentation to reference the correct Windows API call.

PR-URL: nodejs#32156
Refs: nodejs#31732
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
@targos targos removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 25, 2020
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
Backport-PR-URL: nodejs#32166
PR-URL: nodejs#31732
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Apr 28, 2020
Backport-PR-URL: #32166
PR-URL: #31732
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@juanarbol juanarbol deleted the juanarbol/os-version-binding branch January 19, 2021 16:26
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++. os Issues and PRs related to the os subsystem. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants