src: create a getter for kernel version #31732

juanarbol · 2020-02-11T08:13:33Z

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

lib/os.js

mscdex · 2020-02-13T08:47:33Z

lib/os.js

  getOSType: _getOSType,
  getPriority: _getPriority,
  getTotalMem,
  getUserInfo,
  getUptime,
  isBigEndian,
+  getOSVersion,


Suggested change

getOSVersion,

Thanks for the review sr!

juanarbol · 2020-02-13T18:47:47Z

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.

addaleax · 2020-02-13T18:52:57Z

src/node_os.cc

+                               "release",
+                               NewStringType::kNormal).ToLocalChecked(),
+                               release_str)
+           .FromJust();


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:

It removes the need to create strings for the keys in C++, and

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]

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

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]

@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

hola.js

src/node_os.cc

test/parallel/test-os.js

juanarbol · 2020-03-04T15:02:00Z

Is this a semver-minor thing?

nodejs-github-bot · 2020-03-04T16:38:28Z

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

PR-URL: #31732 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>

addaleax · 2020-03-09T11:53:53Z

Landed in c1e6725 🎉

MylesBorins · 2020-03-09T23:03:27Z

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

juanarbol · 2020-03-10T01:04:43Z

MEEEEEEE ME ME ME ME ME MEEEEEEE, I want to

MylesBorins · 2020-03-10T01:45:43Z

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

Backport-PR-URL: #32166 PR-URL: #31732 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>

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

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

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 <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Bartosz Sosnowski <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>

Backport-PR-URL: nodejs#32166 PR-URL: nodejs#31732 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>

Backport-PR-URL: #32166 PR-URL: #31732 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>

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

mscdex reviewed Feb 11, 2020

View reviewed changes

lib/os.js Outdated Show resolved Hide resolved

bnoordhuis reviewed Feb 11, 2020

View reviewed changes

lib/os.js Outdated Show resolved Hide resolved

mscdex reviewed Feb 13, 2020

View reviewed changes

juanarbol force-pushed the juanarbol/os-version-binding branch from 5b7b562 to 654e9d6 Compare February 13, 2020 18:42

addaleax reviewed Feb 13, 2020

View reviewed changes

juanarbol force-pushed the juanarbol/os-version-binding branch from 654e9d6 to 707bef9 Compare February 13, 2020 18:57

devsnek reviewed Feb 13, 2020

View reviewed changes

lib/os.js Show resolved Hide resolved

mscdex reviewed Feb 13, 2020

View reviewed changes

hola.js Outdated Show resolved Hide resolved

juanarbol force-pushed the juanarbol/os-version-binding branch from 707bef9 to bed81ff Compare February 15, 2020 00:01

juanarbol commented Feb 15, 2020

View reviewed changes

src/node_os.cc Outdated Show resolved Hide resolved

addaleax approved these changes Feb 15, 2020

View reviewed changes

src/node_os.cc Outdated Show resolved Hide resolved

src/node_os.cc Outdated Show resolved Hide resolved

src: create a getter for kernel version

1e1bcca

juanarbol force-pushed the juanarbol/os-version-binding branch from bed81ff to 1e1bcca Compare February 15, 2020 00:43

juanarbol added 2 commits February 14, 2020 19:51

fixup

40f7086

fixup

6789e0b

mscdex reviewed Mar 3, 2020

View reviewed changes

test/parallel/test-os.js Show resolved Hide resolved

fixup

03f6681

addaleax added the semver-minor PRs that contain new features and should be released in the next minor version. label Mar 4, 2020

addaleax approved these changes Mar 4, 2020

View reviewed changes

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

jasnell approved these changes Mar 4, 2020

View reviewed changes

addaleax pushed a commit that referenced this pull request Mar 9, 2020

src: create a getter for kernel version

c1e6725

PR-URL: #31732 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>

addaleax closed this Mar 9, 2020

cjihrig mentioned this pull request Mar 9, 2020

doc: fix os.version() Windows API #32156

Merged

3 tasks

MylesBorins added the backport-requested-v13.x label Mar 9, 2020

juanarbol mentioned this pull request Mar 10, 2020

[v13.x backport] src: create a getter for kernel version #32166

Closed

4 tasks

MylesBorins added backported-to-v13.x and removed backport-requested-v13.x labels Mar 10, 2020

MylesBorins pushed a commit that referenced this pull request Mar 10, 2020

src: create a getter for kernel version

258a80d

Backport-PR-URL: #32166 PR-URL: #31732 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>

MylesBorins mentioned this pull request Mar 10, 2020

v13.11.0 proposal #32185

Merged

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

src: create a getter for kernel version

ecf359c

Backport-PR-URL: nodejs#32166 PR-URL: nodejs#31732 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>

targos pushed a commit that referenced this pull request Apr 28, 2020

src: create a getter for kernel version

394487e

Backport-PR-URL: #32166 PR-URL: #31732 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>

targos mentioned this pull request May 2, 2020

v12.17.0 release proposal #33197

Merged

juanarbol deleted the juanarbol/os-version-binding branch January 19, 2021 16:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: create a getter for kernel version #31732

src: create a getter for kernel version #31732

juanarbol commented Feb 11, 2020 •

edited

Loading

mscdex Feb 13, 2020

juanarbol Feb 13, 2020

juanarbol commented Feb 13, 2020

addaleax Feb 13, 2020 •

edited

Loading

addaleax Feb 13, 2020

santigimeno Feb 13, 2020 •

edited

Loading

addaleax Feb 14, 2020

juanarbol commented Mar 4, 2020

nodejs-github-bot commented Mar 4, 2020

addaleax commented Mar 9, 2020

MylesBorins commented Mar 9, 2020

juanarbol commented Mar 10, 2020

MylesBorins commented Mar 10, 2020

	Local<Value> ret[] = {
	proxy->GetTarget(),
	proxy->GetHandler()
	};

	args.GetReturnValue().Set(
	Array::New(args.GetIsolate(), ret, arraysize(ret)));

src: create a getter for kernel version #31732

src: create a getter for kernel version #31732

Conversation

juanarbol commented Feb 11, 2020 • edited Loading

Checklist

mscdex Feb 13, 2020

Choose a reason for hiding this comment

juanarbol Feb 13, 2020

Choose a reason for hiding this comment

juanarbol commented Feb 13, 2020

addaleax Feb 13, 2020 • edited Loading

Choose a reason for hiding this comment

addaleax Feb 13, 2020

Choose a reason for hiding this comment

santigimeno Feb 13, 2020 • edited Loading

Choose a reason for hiding this comment

addaleax Feb 14, 2020

Choose a reason for hiding this comment

juanarbol commented Mar 4, 2020

nodejs-github-bot commented Mar 4, 2020

addaleax commented Mar 9, 2020

MylesBorins commented Mar 9, 2020

juanarbol commented Mar 10, 2020

MylesBorins commented Mar 10, 2020

juanarbol commented Feb 11, 2020 •

edited

Loading

addaleax Feb 13, 2020 •

edited

Loading

santigimeno Feb 13, 2020 •

edited

Loading