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

Refactor RealEnvStore methods to use uv_os_getenv/setenv/unsetenv #27211

Closed
joyeecheung opened this issue Apr 13, 2019 · 3 comments
Closed

Refactor RealEnvStore methods to use uv_os_getenv/setenv/unsetenv #27211

joyeecheung opened this issue Apr 13, 2019 · 3 comments
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. good first issue Issues that are suitable for first-time contributors. os Issues and PRs related to the os subsystem. process Issues and PRs related to the process subsystem.

Comments

@joyeecheung
Copy link
Member

joyeecheung commented Apr 13, 2019 •
edited

From a glance I think the uv_os_* methods are adequate to replace our conditional switches in the RealEnvStore methods.

I think this is a good first issue if you are familiar with C++ and has some idea on how to use libuv methods, but you may need to read into the libuv implementation to make sure the refactor does not change the functionality.

Pointers:

node/src/node_env_var.cc

Line 71 in dfd7e99

const char* val = getenv(*key);

node/src/node_env_var.cc

Line 107 in dfd7e99

setenv(*key, *val, 1);

node/src/node_env_var.cc

Line 123 in dfd7e99

if (getenv(*key)) return 0;

node/src/node_env_var.cc

Line 146 in dfd7e99

unsetenv(*key);

docs:

http://docs.libuv.org/en/v1.x/misc.html?highlight=uv_os_getenv#c.uv_os_getenv
http://docs.libuv.org/en/v1.x/misc.html?highlight=uv_os_getenv#c.uv_os_setenv
http://docs.libuv.org/en/v1.x/misc.html?highlight=uv_os_getenv#c.uv_os_unsetenv
@joyeecheung joyeecheung added good first issue Issues that are suitable for first-time contributors. os Issues and PRs related to the os subsystem. process Issues and PRs related to the process subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. labels Apr 13, 2019
@joyeecheung
Copy link
Member Author

joyeecheung commented Apr 13, 2019 •
edited

Prior work: #14641

It seems easier to refactor uv_os_setenv and uv_os_unsetenv, uv_os_getenv is trickier due to how the buffer allocation is handled - the caller needs to retry when the allocated buffer is not large enough.

@devasci
Copy link
Contributor

devasci commented Apr 15, 2019

Hi, It seems to be a good item for me as a first timer. I will work one this and will share the updates soon.

@devasci
Copy link
Contributor

devasci commented Apr 19, 2019

Modified as Joyee Cheung suggested and tested by following contributors guide.

Created pull request: #27310

@joyeecheung joyeecheung mentioned this issue Aug 20, 2019
Closed
3 tasks
addaleax pushed a commit to devasci/node that referenced this issue Aug 20, 2019
@devasci @addaleax
src: modified RealEnvStore::Get method to use libuv functions
1501433 
Modified RealEnvStore::Get - removed os switch statements
and replaced logic to use libuv uv_getos_env method.

Fixes: nodejs#27211
Refs: http://docs.libuv.org/en/v1.x/misc.html
addaleax pushed a commit to devasci/node that referenced this issue Aug 20, 2019
@devasci @addaleax
src: modified RealEnvStore methods to use libuv functions
eef9b8b 
Modified RealEnvStore::Get, Set, Query and Delete methods
to use libuv methods environment variables operations instead
of using os specific logic and switches.

Fixes: nodejs#27211
Refs: http://docs.libuv.org/en/v1.x/misc.html
addaleax pushed a commit to devasci/node that referenced this issue Aug 20, 2019
@devasci @addaleax
src: Modified src/node_env_var.cc as per Joyee Cheung’s review comments
7db9f03 
Below review comments are taken care in this submission:
1. followed snake case naming convention for local variables
2. dropped sizeof(char) since it is obvious value 1
3. Used MaybeLocal<String> instead of Local<String> to check
for
   empty strings and throw exception if empty.

Fixes: nodejs#27211
Refs: https://v8docs.nodesource.com/node-4.8/annotated.html
addaleax pushed a commit to devasci/node that referenced this issue Aug 20, 2019
@devasci @addaleax
src: Modified to use 2 space for indentation.
6ae79f7 
used 2 spaces instead of 4 for indentation

Fixes: nodejs#27211
Refs: https://v8docs.nodesource.com/node-4.8/annotated.html
addaleax pushed a commit to devasci/node that referenced this issue Aug 20, 2019
@devasci @addaleax
src: refactor RealEnvStore methods - review comments fixing - 1
0271ee1 
Below review comments by Anna Henningsen are taken care:
	1. avoided Yoda style comparisons
	2. used MaybeStackBuffer instead of raw char pointers
	3. used MaybeLocal<String> to inspect for empty string value
            and then raise exception and return empty Local<String> handle.
            (Changing return type of RealEnvStore::Get method to MaybeLocal<String>
              is pending, and planning to submit in next commit)

Fixes: nodejs#27211
Refs: nodejs#27310 (comment)
addaleax pushed a commit to devasci/node that referenced this issue Aug 20, 2019
@devasci @addaleax
src: refactor RealEnvStore methods - review comments fixing - 2
4d51c93 
Modified KVStore::Get return type to MaybeLocal and also modified
RealEnvStore::Get, MapKVStore::Get respectively.

Fixes: nodejs#27211
Refs: nodejs#27310 (comment)
addaleax pushed a commit to devasci/node that referenced this issue Aug 20, 2019
@devasci @addaleax
src: refactor RealEnvStore methods - review comments fixing - 2a
1d0aa6e 
Removed exception throwing if value is empty, since the same is handled
in caller and returned immediately.

Fixes: nodejs#27211
Refs: nodejs#27310 (comment)
addaleax pushed a commit to devasci/node that referenced this issue Aug 20, 2019
@devasci @addaleax
src: refactor RealEnvStore methods - review comments fixing - 3
b40d71e 
1. All tests were failing due to the exception raising from within 

   RealEnvStore::Get method, removed the exception raising since 

   the caller are checking for empty handles and taking necessary 
actions.
2. Used MayLocal<String>’s ToLocalChecked() function instead of
FromMaybe().

Fixes: nodejs#27211
Refs: nodejs#27310 (comment)
addaleax pushed a commit to devasci/node that referenced this issue Aug 20, 2019
@devasci @addaleax
src: refactor RealEnvStore methods - Joyee Cheung’s review remarks fixed
88fedfc 
Modified an if statment and formatted the code as per Joyee Cheung's
review comments

Fixes: nodejs#27211
Refs: nodejs#27310 (comment)
Refs: nodejs#27310 (comment)
addaleax pushed a commit to devasci/node that referenced this issue Aug 20, 2019
@devasci @addaleax
src: handle windows hidden/read-only env keys in set/query methods
614f357 
All windows tests were failing after previous commit, as per
Joyee Cheung suggestion modified to inspect the first character
of env variable key’s on windows machine for the character ‘=‘
and skip if the key contains. Since these keys are hidden/read-only
env vars in windows machines.

Fixes: nodejs#27211
Refs: nodejs#27310 (comment)
targos pushed a commit that referenced this issue Sep 20, 2019
@devasci @targos
src: modified RealEnvStore methods to use libuv functions
981ec7d 
Modified RealEnvStore::Get, Set, Query and Delete methods
to use libuv methods environment variables operations instead
of using os specific logic and switches.

Fixes: #27211
Refs: http://docs.libuv.org/en/v1.x/misc.html

PR-URL: #27310
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
BridgeAR pushed a commit that referenced this issue Sep 25, 2019
@devasci @BridgeAR
src: modified RealEnvStore methods to use libuv functions
4a5ba60 
Modified RealEnvStore::Get, Set, Query and Delete methods
to use libuv methods environment variables operations instead
of using os specific logic and switches.

Fixes: #27211
Refs: http://docs.libuv.org/en/v1.x/misc.html

PR-URL: #27310
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. good first issue Issues that are suitable for first-time contributors. os Issues and PRs related to the os subsystem. process Issues and PRs related to the process subsystem.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@joyeecheung @devasci