WIP: unix,win: add uv_homedir() by cjihrig · Pull Request #350 · libuv/libuv

cjihrig · 2015-05-12T13:28:32Z

This is a first pass at implementing a homedir() function, per the discussion in nodejs/node#1670.

@piscisaureus I'm running into a problem on the Windows side, where including shlobj.h (needed for SHGetKnownFolderPath()) is causing a ton of compiler errors. Any assistance would be much appreciated.

Also related to #11.

saghul · 2015-05-12T13:46:31Z

Is this the only thing we need/want from struct passwd? I guess the username / user ID would also be nice, wouldn't it?

saghul · 2015-05-12T13:48:21Z

We can now document functions with automagic links to man pages:

":man:getaddrinfo(3)"

cjihrig · 2015-05-12T14:10:53Z

Is this the only thing we need/want from struct passwd? I guess the username / user ID would also be nice, wouldn't it?

Yes, it would be nice, but we are limited by the Windows side. Separate calls are needed for all of these things - SHGetKnownFolderPath() gets the home directory, but we need to call GetUsernameW() to get the username. I'm open to doing that if that's the way everyone else wants to go.

saghul · 2015-05-12T14:32:45Z

Yes, it would be nice, but we are limited by the Windows side. Separate calls are needed for all of these things - SHGetKnownFolderPath() gets the home directory, but we need to call GetUsernameW() to get the username. I'm open to doing that if that's the way everyone else wants to go.

I don't know, just wondering :-) Maybe some uv_passwd_t, with the common subset of things would be a good thing. On Unix we could fill in more things than on Windows.

As a side note, Python does this: https://docs.python.org/2/library/getpass.html#getpass.getuser that is, it reads the environment variables to get the username. Doesn't sound very reliable though.

cjihrig · 2015-05-12T18:30:26Z

@saghul I updated the PR based on your comments, minus moving to the uv_passwd_t format. Regarding Python, I think that we should avoid relying on environment variables if possible.

cjihrig · 2015-05-15T19:29:47Z

@piscisaureus ping

piscisaureus · 2015-05-15T19:56:26Z

use len - 1 because otherwise adding the null terminator (couple lines below) could cause a buffer overrun.

I'd prefer to use WideCharToMultiByte instead of wcstombs like we do everywhere in libuv.

cjihrig · 2015-05-15T19:59:49Z

@piscisaureus sorry I should have added this to my ping. Please see my message when I originally opened the PR. I'm having problems with Windows and including shlobj.h. The Windows code isn't ready for a thorough review yet.

piscisaureus · 2015-05-15T20:00:15Z

What's implied here is that the returned length doesn't include the null terminator.
I have no particular opinion on whether that's good or bad, but please ensure that this API behaves the same as the unix version, and be consistent with other path-returning functions (e.g. uv_cwd())

piscisaureus · 2015-05-15T20:07:15Z

I'm having problems with Windows and including shlobj.h

Just make sure shlobj.h is included after windows.h.
Since windows.h is pulled in by uv.h you want to move it after uv.h.

piscisaureus · 2015-05-15T20:09:22Z

Some other comments:

The windows implementation should live in util.c (where utility functions live) and not core.c (which contains the core event loop driver).
It'd be good to make sure trailing (back)slashes are included (or excluded) consistently across supported platforms. This has caused issues in the past with uv_cwd().

cjihrig · 2015-05-16T00:56:08Z

@piscisaureus thank you for the help. I've updated the PR based on your comments. The unix and windows branches both ignore the null terminator when returning the length. Also, in my testing I didn't encounter any trailing slashes on the home directories. The SHGetKnownFolderPath() documentation explicitly states that a trailing slash is not included.

piscisaureus · 2015-05-16T01:09:50Z

@cjihrig Great.

The windows bits lgtm.

I guess @bnoordhuis or @saghul could review the unix implementation. I'm a bit surprised that looking up the home dir requires retrieving the username first - looks like getpw(3) just provides the right information - but then I may be overlooking something.

saghul · 2015-05-16T07:56:48Z

I'm traveling ATM, I'll have a look on Monday, if Ben doesn't beat me to it
:-)
On May 16, 2015 03:09, "Bert Belder" notifications@github.com wrote:

@cjihrig https://github.com/cjihrig Great.

The windows bits lgtm. I guess @bnoordhuis https://github.com/bnoordhuis
or @saghul https://github.com/saghul could review the windows bits -
I'm a bit surprised that looking up the home dir requires retrieving the
username first - looks like getpw(3) just provides the right information

but then I'm may be overlooking something.

—
Reply to this email directly or view it on GitHub
#350 (comment).

bnoordhuis · 2015-05-16T21:33:01Z

This looks incorrect to me. Most platforms leave _POSIX_LOGIN_NAME_MAX at the POSIX default of 9 (8 characters + zero byte) even though their real maximum is much larger.

cjihrig · 2015-05-19T21:50:08Z

I don't have much say in how libuv behaves, but to me it seems that truncating is not helpful, and instead it would be better to return UV_ENOBUFS or E2BIG (your call), and set *size to the size required (pass or fail). I guess that would change the current success semantics where *size is set to the strlen() and not the actual size required.

saghul · 2015-05-19T21:52:42Z

Probably unlikely, but we should check GetLastError here, in case it's ERROR_ENVVAR_NOT_FOUND it's not an error, we should just continue. I see you handle it later, but I guess we need to check both times, because threads!

saghul · 2015-05-19T21:57:58Z

None of the other APIs touch len on error. I'd opt for consistency now and perhaps revise in v2.0. Just return E2BIG if the result doesn't fit.

@bnoordhuis but they do!

https://github.com/libuv/libuv/blob/v1.x/src/uv-common.c#L373
https://github.com/libuv/libuv/blob/v1.x/src/unix/pipe.c#L239
https://github.com/libuv/libuv/blob/v1.x/src/win/util.c#L209

cjihrig · 2015-05-20T03:34:48Z

Another round of updates pushed :-)

saghul · 2015-05-20T07:27:15Z

LGTM!

CI: https://jenkins-iojs.nodesource.com/view/libuv/job/libuv+any-pr+multi/94/

piscisaureus · 2015-05-20T07:31:06Z

Great! let's establish some common patterns

saghul · 2015-05-20T08:10:55Z

Build has tons of red, but that has nothing to do with this patch, great work @cjihrig!

saghul · 2015-05-20T08:11:59Z

One last thing (if you're not around I'll add it during merge) can you add a ".. versionadded 1.6.0" here please? :-)

saghul · 2015-05-20T14:20:44Z

Fantastic! Can we get another LGTM? /cc @bnoordhuis, @piscisaureus

bnoordhuis · 2015-05-20T14:58:08Z

I'd rather you stick to getpwuid_r() here. getenv() at least is MT-safe on most platforms if no other threads concurrently modify the environment, but getpwuid() is basically always unsafe.

Oh I missed that during my review. Agreed!

@bnoordhuis @saghul restored getpwuid_r().

bnoordhuis · 2015-05-21T15:29:21Z

LGTM

saghul · 2015-05-21T15:31:05Z

Kewl, I'll merge then.

PR-URL: #350 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com>

saghul · 2015-05-21T15:33:31Z

Great work Colin! Landed in ✨ a62c2d5 ✨

cjihrig · 2015-05-21T15:37:18Z

Thanks Saul/Ben/Bert for working with me through this.

PR-URL: libuv#350 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com>

saghul reviewed May 12, 2015
View reviewed changes

cjihrig force-pushed the homedir branch 5 times, most recently from fe57056 to c98af6d Compare May 12, 2015 17:38

piscisaureus reviewed May 15, 2015
View reviewed changes

cjihrig force-pushed the homedir branch 2 times, most recently from 8280d15 to 9d26496 Compare May 16, 2015 00:51

bnoordhuis reviewed May 16, 2015
View reviewed changes

saghul reviewed May 19, 2015
View reviewed changes

cjihrig force-pushed the homedir branch 4 times, most recently from bf12f74 to c22b2f4 Compare May 20, 2015 03:29

saghul reviewed May 20, 2015
View reviewed changes

cjihrig force-pushed the homedir branch from c22b2f4 to d1920aa Compare May 20, 2015 13:17

bnoordhuis reviewed May 20, 2015
View reviewed changes

unix,win: add uv_os_homedir()

cd661b3

cjihrig force-pushed the homedir branch from d1920aa to cd661b3 Compare May 21, 2015 15:16

saghul pushed a commit that referenced this pull request May 21, 2015

unix,win: add uv_os_homedir()

a62c2d5

PR-URL: #350 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com>

saghul closed this May 21, 2015

cjihrig deleted the homedir branch May 21, 2015 15:37

guworks pushed a commit to guworks/libuv that referenced this pull request Jul 9, 2015

unix,win: add uv_os_homedir()

77799d8

PR-URL: libuv#350 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com>

vtjnash mentioned this pull request Jul 31, 2015

tilde expansion in path names JuliaLang/julia#1136

Closed

skomski pushed a commit to skomski/libuv that referenced this pull request Aug 5, 2015

unix,win: add uv_os_homedir()

40421f8

PR-URL: libuv#350 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com>

skomski pushed a commit to skomski/libuv that referenced this pull request Aug 5, 2015

unix,win: add uv_os_homedir()

1c8088f

PR-URL: libuv#350 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com>

XadillaX mentioned this pull request Mar 7, 2016

Issue about os.homedir() nodejs/node#5582

Closed

cjihrig mentioned this pull request Jun 22, 2019

thread safety of misc utils #2339

Closed

Conversation

cjihrig commented May 12, 2015

Uh oh!

saghul commented May 12, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjihrig commented May 12, 2015

Uh oh!

saghul commented May 12, 2015

Uh oh!

cjihrig commented May 12, 2015

Uh oh!

cjihrig commented May 15, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjihrig commented May 15, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

piscisaureus commented May 15, 2015

Uh oh!

piscisaureus commented May 15, 2015

Uh oh!

cjihrig commented May 16, 2015

Uh oh!

piscisaureus commented May 16, 2015

Uh oh!

saghul commented May 16, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjihrig commented May 19, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saghul commented May 19, 2015

Uh oh!

cjihrig commented May 20, 2015

Uh oh!

saghul commented May 20, 2015

Uh oh!

piscisaureus commented May 20, 2015

Uh oh!

saghul commented May 20, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saghul commented May 20, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bnoordhuis commented May 21, 2015

Uh oh!

saghul commented May 21, 2015

Uh oh!

saghul commented May 21, 2015

Uh oh!

cjihrig commented May 21, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants