Page MenuHomePhabricator

Fallback to getpwuid() in path::home_directory() on Unix.
Needs ReviewPublic

Authored by chfast on Nov 6 2015, 8:14 AM.

Diff Detail

Event Timeline

chfast updated this revision to Diff 39538.Nov 6 2015, 8:14 AM
chfast retitled this revision from to Fallback to getpwuid() in path::home_directory() on Unix..
chfast updated this object.
chfast added a subscriber: llvm-commits.
chfast updated this revision to Diff 40621.Nov 19 2015, 2:38 AM

Refactor unit test helpers. Check if pwh.h is available in every place that requires it.

Gentle ping. This patch includes a small unit test helpers refactoring that I would like to use in other patches.

rafael edited edge metadata.Nov 24 2015, 11:35 AM
rafael added a subscriber: rafael.

Can you give an overview of when the HOME environment variable is not defined?

Cheers,
Rafael

aaron.ballman added inline comments.Nov 24 2015, 12:47 PM
lib/Support/Unix/Path.inc
558

Why 1024?

574

I'm not certain whether Dir is required here or not, but knowing its type would be good. We only use auto when the type is already spelled out, or when it's difficult to spell out (like some iterators).

584

Is getpwuid() more reliable than reading the environment variable?

Can you give an overview of when the HOME environment variable is not defined?

E.g. in llvm-lit testing environment.

lib/Support/Unix/Path.inc
574

I will change that.

584

I don't understand the question. Are you asking which approach is better?

Checking the environmental variable first allows the user change home directory location visible by the application.

chfast added inline comments.Nov 25 2015, 5:57 AM
lib/Support/Unix/Path.inc
558

It is suggested initial buffer size returned by sysconf(_SC_GETPW_R_SIZE_MAX).

aaron.ballman added inline comments.Nov 25 2015, 6:40 AM
lib/Support/Unix/Path.inc
558

Ah, good to know, thank you!

584

I was under the impression (which may be incorrect) that getpwuid() was a safer, more reliable way to get the home directory, when the API is available, and the environment variable is more of a backup plan. If that is true, it would make sense to call getHomeDirFromPasswd() first, and fallback to getenv().

E.g. in llvm-lit testing environment.

Isn't the intention precisely to avoid tests creating files in the
home directory or being dependent on global config entries?

Cheers,
Rafael

chfast added a comment.Jan 7 2016, 8:47 AM

E.g. in llvm-lit testing environment.

Isn't the intention precisely to avoid tests creating files in the
home directory or being dependent on global config entries?

Cheers,
Rafael

I'm not sure.

  1. Temp directory does not fallback to $HOME.
  2. Setting $HOME to "" might be a workaround.
  3. On Windows Windows API is used to find out the user's home dir in case information from environment variables are not enough.

I don't really need this change, so if you believe it is an overkill, I'm happy to drop it.

Added authors/reviewers of original patch adding home_directory() function in http://reviews.llvm.org/D2199

Do you have any opinion about my patch? I don't insist on it to be merged.

yaron.keren resigned from this revision.Mar 30 2017, 12:09 PM
espindola edited reviewers, added: espindola; removed: rafael.Mar 15 2018, 10:57 AM