-
Notifications
You must be signed in to change notification settings - Fork 593
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.
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
Use lsof to get cwd of shell when /proc/pid/cwd isn't available #436
Conversation
I've checked the sources of lsof for OS X but the code is way too low-level for Geany and also too messy. Using lsof directly seems to be cleaner and more platform-independent. |
How often is the cwd test run? I thought it was every command? I was gonna suggest lsof or procstat, but then realised that spawning another command everytime a command is run is rather expensive. |
By the way, I'm a little puzzled by the vte_restart() code - we kill the shell, set pid to 0, but who restarts it? And cannot we get the pid in this case? @elextr It's called on shell restart (ctrl+d or Geany termination) and also when setting directory from outside by e.g. "follow path of the current file" and others. I don't think it will have any noticeable performance impact. |
Eh, haven't noticed the additional 'f' record which is present on PC BSD. Relax the checks a bit so we can skip records we are not interested in (according to docs the 'p' record should precede the rest so we can hopefully check its presence in the 'n' record). |
if (utils_spawn_sync(NULL, argv, NULL, G_SPAWN_SEARCH_PATH, | ||
NULL, NULL, &stdout, NULL, &status, NULL)) | ||
{ | ||
if (status == 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WEXITSTATUS(status) == 0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uh, I might sometimes check the documentation...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, I guess the real test her needs to be something like WIFEXITED(status) && WEXITSTATUS(status == 0)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would assume that successful g_spawn_sync() guarantees that but better to be on the safe side.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not, the returned exit_status
is the raw value from waitpid()
. So the child might have died of any reason, including being killed, crashing, or exiting normally.
I have the same concerns as @elextr, but if it's only used if all else failed, why not. But then, what about doing it like xfce4-terminal (the source of our original code) and try BSD's Linux compatibility layers first? |
|
lsof without any parameters is usually quite slow but it seems instant with the parameters used in the patch so I think there won't be any negative performance impact even on systems without /proc. The lsof code https://opensource.apple.com/source/lsof/lsof-53/lsof/proc.c on darwin looks quite platform-specific, full of ifdefs and I'm absolutely not sure if it will work on other BSD systems so using lsof itself as an interface seems to be a cleaner option (though it might not be installed everywhere).
Shouldn't hurt, I will add them (without testing ;-). Will fix the rest of the issues too. |
I have already spent my portion of "who calls what" in the 15kloc vte.c |
lsof can be used to get cwd of a process and it's available on basically every UNIX. In addition, it offers the "output for other programs" mode which is kind of standardized and shouldn't change in the future. I have tested the lsof command on linux, PC BSD and OS X and both the parameters and output are mostly identical everywhere (on PC BSD there's an additional 'f' field). The patch performs some checks if the output corresponds to what we expect. In addition, it tries to find procfs on alternative paths under various BSD systems (code taken from xfce terminal).
I tried to understand it, and it's actually in |
Yes, sorry, linked a wrong file. I was worried the actual implementation is in the darwin directory so it might be something completely different on other BSD systems (haven't checked). |
Just to resolve this confusion: the restart of the shell was previously done in Geany and so the pid got set properly. But then we removed the start code because of a VTE bug and so pid kept 0 and broke other things :(. JFYI, it won't help this issue. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good apart the comment, although I didn't actually test it on anything but my Linux :)
gint status = 0; | ||
gchar *stdout = NULL; | ||
gchar *pid_str = g_strdup_printf("%d", pid); | ||
gchar *argv[] = {"lsof", "-a", "-d", "cwd", "-F", "n", "-p", pid_str, NULL}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally you'd make this a const gchar*
array, so that literals can be constant; and in the call below just cast up to gchar**
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utils_spawn_sync
calls spawn_sync
which on Linux eventually calls g_spawn_async
which specifies the array as gchar**
, not a const
in sight, so it may be a pain to try to const
ize this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note the exec
family of system calls takes char* const argv[]
so g_spawn
is looser than needed.
I'd say this feature (i.e. setting the working directory of vte to the last used directory when Geany is re-launched) isn't worth adding this slightly ugly code. Unless people start complaining about this, I'd just leave this pull request open. |
Nobody has complained in the last 5 years so closing. |
lsof can be used to get cwd of a process and it's available
on basically every UNIX. In addition, it offers the "output for
other programs" mode which is kind of standardized and
shouldn't change in the future. I have tested the lsof command
on linux, PC BSD and OS X and both the parameters and
output are identical everywhere. In addition, the patch
performs some checks if the output corresponds to what we
expect.