[Lldb-commits] [PATCH] D68316: [Host] Return the user's shell from GetDefaultShell

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.


================
Comment at: lldb/lit/Host/TestCustomShell.test:5
+# RUN: SHELL=bogus %lldb %t.out -b -o 'run' 2>&1 | FileCheck %s
+# CHECK: error: shell expansion failed
----------------
JDevlieghere wrote:
> JDevlieghere wrote:
> > friss wrote:
> > > Is there a reliable way to check that the expansion we get in lldb matches the one in the shell? For example, could we have the program dump its arguments once without lldb and match them against the lldb output?
> > > 
> > > I guess the zsh example that errors out in your description makes this hard?  
> > You could run lldb-argdumper under lldb and compare the output to running it under different shells. Do you think it's reasonable to assume that at least `/bin/bash` and `/bin/zsh` are available?
> Assuming we can find something that expands differently without throwing an error in bash and zsh.
I don't think you can assume zsh is universally available. If you wanted to do something like, the safest option would be to write a mock shell, which implements some dumb substitution (s/FOO/BAR/) and otherwise forwards to the real shell. But, I wouldn't say that is required, as that is not even testing what this patch is changing. A simpler but still interesting test might be to unset the SHELL variable to ensure that the getpwuid path is at least executed (as one can't really check it's result reasonably).


================
Comment at: lldb/source/Host/posix/HostInfoPosix.cpp:119
+    return FileSpec(v);
+  if (const char *v = ::getpwuid(::geteuid())->pw_shell)
+    return FileSpec(v);
----------------
friss wrote:
> the dereference seems a little careless. getpwuid can fail and return NULL.
Also, getpwuid is not thread safe. There's already code in this very file which chooses between getpwuid and getpwuid_r, but it is hidden inside `PosixUserIDResolver::DoGetUserName`. If you factor that out into a helper function, you'll be able to always use the best available API.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68316/new/

https://reviews.llvm.org/D68316