This is an archive of the discontinued LLVM Phabricator instance.

Fix llvm::sys::path::home_directory on unix to get path from passwd.pw_dir when the $HOME variable isn't set
AbandonedPublic

Authored by ki.stfu on Feb 9 2015, 11:17 AM.

Details

Reviewers
chandlerc
Summary

This patch adds alternative way to get the user's home directory.

Diff Detail

Event Timeline

ki.stfu updated this revision to Diff 19601.Feb 9 2015, 11:17 AM
ki.stfu retitled this revision from to Fix llvm::sys::path::home_directory on unix to get path from passwd.pw_dir when the $HOME variable isn't set.
ki.stfu updated this object.
ki.stfu edited the test plan for this revision. (Show Details)
ki.stfu set the repository for this revision to rL LLVM.
ki.stfu added a subscriber: Unknown Object (MLST).
ki.stfu updated this revision to Diff 19603.Feb 9 2015, 11:58 AM
ki.stfu removed rL LLVM as the repository for this revision.

Use thread-safe version of getpwuid.

ki.stfu set the repository for this revision to rL LLVM.
ki.stfu added a subscriber: chandlerc.

Hello Chandler,

RE: Why is this useful?

It tries to get home_direictory by all ways. It's useful in case when HOME wasn't set (that was in my case).

RE: The documentation for getpwuid specifically suggests that applications should merely inspect the HOME environment variable.

I didn't see any references to HOME variable. (see http://pubs.opengroup.org/onlinepubs/009695399/functions/getpwuid.html)

RE: If this is truly necessary, it should use getpwuid_r to be thread-safe, and it should probably be protected by compatibility macros as I don't know which of the unix variants we support implement this functionality. But maybe it is sufficiently widely available across BSD, Linux, Solaris, Unix, and Mac OS variants....

I have updated my patch (now it uses thread-safe function) but I don't know how to protect the inclusion of pwd.h (HAVE_PWD_H is absent).

Of course I can setup HOME (using getpwuid()) on start but I think that path::home_directory() should do it for me.

Thanks,
Ilia

Hello Chandler,

RE: But what platform is it that we're hypothetically supporting? All the BSD and Linux flavors I know of rely heavily on the HOME environment variable. All the build bots I know of test under that assumption. I'm worried about adding code to support another variant that we have essentially no way of testing and no knowledge of why it was important.

I'm working on OS X but in this case I launch lldb from another process. Perhaps I forgot to pass HOME variable in execve(). I didn't know that lldb (and llvm) uses that variable. I think we should fix it to avoid such problems in the future (or warn that HOME variable isn't set).

Thanks,
Ilia

Friendly ping

A few comments:

  • Use clang-format or manually follow the rules defined here: http://llvm.org/docs/CodingStandards.html
  • You are using a VLA, this is not C++ standard I believe.
  • At least add a clear comment on what you are doing and why (explain why getenv("HOME") might fail.

Thanks.

ki.stfu abandoned this revision.Feb 10 2015, 10:37 PM

Hello Chandler,

RE: It's really unclear to me that we should support this at all rather. I feel like we're adding complexity we really don't need.

Ok. I don't mind. I had found an error and can fix it on my side.

@joker.eph,@chandlerc, thank you for your time.

Thanks,
Ilia

In D7515#121843, @joker.eph wrote:

FWIW, it seems to me that Python is doing it this way: https://github.com/enthought/Python-2.7.3/blob/master/Lib/posixpath.py

Good note.

In D7515#121843, @joker.eph wrote:

FWIW, it seems to me that Python is doing it this way: https://github.com/enthought/Python-2.7.3/blob/master/Lib/posixpath.py

As you see my fix does the same thing.

Are you interested in this patch? If so I can fix your remarks