Am 18.07.2014 12:49, schrieb Duy Nguyen: > On Fri, Jul 18, 2014 at 6:03 AM, Karsten Blees <karsten.blees@xxxxxxxxx> wrote: >> Am 17.07.2014 19:05, schrieb René Scharfe: >>> Am 17.07.2014 14:45, schrieb Nguyễn Thái Ngọc Duy: >> [...] >>> "These routines have traditionally been used by programs to save the >>> name of a working directory for the purpose of returning to it. A much >>> faster and less error-prone method of accomplishing this is to open the >>> current directory (.) and use the fchdir(2) function to return." >>> >> >> fchdir() is part of the POSIX-XSI extension, as is realpath(). So why not >> use realpath() directly (which would also be thread-safe)? > > But realpath still needs a given buffer (of PATH_MAX at least again). > Unless you pass a NULL pointer as "resolved_name", then Linux can > allocate the buffer but that's implementation specific [1]. I guess we > can make a wrapper "git_getcwd" that returns a new buffer. The default > implementation returns one of PATH_MAX chars, certain platforms like > Linux can use realpath (or something else) to return a longer path? > > [1] http://pubs.opengroup.org/onlinepubs/009695399/functions/realpath.html realpath() can be used to implement the whole of real_path_internal(), IIUC, so there would be no need to change the current directory anymore. The realpath(3) manpage for Linux mentions that behaviour of realpath() with resolved_path being NULL is not standardized by POSIX.1-2001 but by POSIX.1-2008. At least Linux, OpenBSD and FreeBSD seem to support that, based on their manpages. Here's the standard doc: http://pubs.opengroup.org/onlinepubs/9699919799/functions/realpath.html >> For non-XSI-compliant platforms, we could keep the current implementation. >> Or re-implement a thread-safe version, e.g. applying resolve_symlink() from >> lockfile.c to all path components. >> >> >> If I may bother you with the Windows point of view: >> >> There is no fchdir(), and I'm pretty sure open(".") won't work either. >> >> fchdir() could be emulated using GetFileInformationByHandleEx(FileNameInfo). >> realpath() is pretty much what GetFinalPathNameByHandle() does. However, >> both of these APIs require Windows Vista. >> >> Opening a handle to a directory can be done using FILE_FLAG_BACKUP_SEMANTICS, >> which AFAICT MSVCRT.dll's open() implementation does _not_ do (could be >> emulated in our mingw_open() wrapper, though). >> >> ...lots of work for little benefit, I would think. >> > > We could wrap this "get cwd, cd back" pattern as well. So "save_cwd" > returns an opaque pointer, "go_back" takes the pointer, (f)chdir back > and free the pointer if needed. On platforms that support fchdir, it > can be used, else we fall back to chdir. I think there are only four > places that follow this pattern, here, setup.c (.git discovery), git.c > (restore_env) and unix-socket.c. Enough call sites to make it worth > the effort? On a cursory look I didn't find any more potential users. Something like this? Applying it to setup.c looks difficult to impossible, though. --- Makefile | 8 ++++++++ abspath.c | 10 ++++++---- cache.h | 1 + git.c | 9 +++++---- save-cwd-fchdir.c | 25 +++++++++++++++++++++++++ save-cwd.c | 22 ++++++++++++++++++++++ save-cwd.h | 3 +++ unix-socket.c | 11 ++++++----- 8 files changed, 76 insertions(+), 13 deletions(-) create mode 100644 save-cwd-fchdir.c create mode 100644 save-cwd.c create mode 100644 save-cwd.h diff --git a/Makefile b/Makefile index 840057c..ecf3129 100644 --- a/Makefile +++ b/Makefile @@ -32,6 +32,8 @@ all:: # # Define HAVE_ALLOCA_H if you have working alloca(3) defined in that header. # +# Define HAVE_FCHDIR if you have fchdir(2). +# # Define NO_CURL if you do not have libcurl installed. git-http-fetch and # git-http-push are not built, and you cannot use http:// and https:// # transports (neither smart nor dumb). @@ -1118,6 +1120,12 @@ ifdef HAVE_ALLOCA_H BASIC_CFLAGS += -DHAVE_ALLOCA_H endif +ifdef HAVE_FCHDIR + COMPAT_OBJS += save-cwd-fchdir.o +else + COMPAT_OBJS += save-cwd.o +endif + ifdef NO_CURL BASIC_CFLAGS += -DNO_CURL REMOTE_CURL_PRIMARY = diff --git a/abspath.c b/abspath.c index ca33558..f49bacc 100644 --- a/abspath.c +++ b/abspath.c @@ -41,7 +41,7 @@ static const char *real_path_internal(const char *path, int die_on_error) * here so that we can chdir() back to it at the end of the * function: */ - char cwd[1024] = ""; + struct saved_cwd *cwd = NULL; int buf_index = 1; @@ -80,7 +80,9 @@ static const char *real_path_internal(const char *path, int die_on_error) } if (*buf) { - if (!*cwd && !getcwd(cwd, sizeof(cwd))) { + if (!cwd) + cwd = save_cwd(); + if (!cwd) { if (die_on_error) die_errno("Could not get current working directory"); else @@ -142,8 +144,8 @@ static const char *real_path_internal(const char *path, int die_on_error) retval = buf; error_out: free(last_elem); - if (*cwd && chdir(cwd)) - die_errno("Could not change back to '%s'", cwd); + if (cwd && restore_cwd(cwd)) + die_errno("Could not change back to the original working directory"); return retval; } diff --git a/cache.h b/cache.h index 92fc9f1..5b4e9cd 100644 --- a/cache.h +++ b/cache.h @@ -7,6 +7,7 @@ #include "advice.h" #include "gettext.h" #include "convert.h" +#include "save-cwd.h" #include SHA1_HEADER #ifndef git_SHA_CTX diff --git a/git.c b/git.c index 5b6c761..946cc82 100644 --- a/git.c +++ b/git.c @@ -20,7 +20,7 @@ const char git_more_info_string[] = static struct startup_info git_startup_info; static int use_pager = -1; -static char orig_cwd[PATH_MAX]; +static struct saved_cwd *orig_cwd; static const char *env_names[] = { GIT_DIR_ENVIRONMENT, GIT_WORK_TREE_ENVIRONMENT, @@ -36,7 +36,8 @@ static void save_env(void) if (saved_environment) return; saved_environment = 1; - if (!getcwd(orig_cwd, sizeof(orig_cwd))) + orig_cwd = save_cwd(); + if (!orig_cwd) die_errno("cannot getcwd"); for (i = 0; i < ARRAY_SIZE(env_names); i++) { orig_env[i] = getenv(env_names[i]); @@ -48,8 +49,8 @@ static void save_env(void) static void restore_env(void) { int i; - if (*orig_cwd && chdir(orig_cwd)) - die_errno("could not move to %s", orig_cwd); + if (orig_cwd && restore_cwd(orig_cwd)) + die_errno("could not move to the original working directory"); for (i = 0; i < ARRAY_SIZE(env_names); i++) { if (orig_env[i]) setenv(env_names[i], orig_env[i], 1); diff --git a/save-cwd-fchdir.c b/save-cwd-fchdir.c new file mode 100644 index 0000000..5327932 --- /dev/null +++ b/save-cwd-fchdir.c @@ -0,0 +1,25 @@ +#include "cache.h" + +struct saved_cwd { + int cwd_fd; +}; + +struct saved_cwd *save_cwd(void) +{ + struct saved_cwd *cwd = xmalloc(sizeof(*cwd)); + cwd->cwd_fd = open(".", O_RDONLY); + if (cwd->cwd_fd < 0) { + free(cwd); + return NULL; + } + return cwd; +} + +int restore_cwd(struct saved_cwd *cwd) +{ + int rc = fchdir(cwd->cwd_fd); + if (!rc) + rc = close(cwd->cwd_fd); + free(cwd); + return rc; +} diff --git a/save-cwd.c b/save-cwd.c new file mode 100644 index 0000000..1864e9f --- /dev/null +++ b/save-cwd.c @@ -0,0 +1,22 @@ +#include "cache.h" + +struct saved_cwd { + char cwd[PATH_MAX]; +}; + +struct saved_cwd *save_cwd(void) +{ + struct saved_cwd *cwd = xmalloc(sizeof(*cwd)); + if (!getcwd(cwd->cwd, sizeof(cwd->cwd))) { + free(cwd); + return NULL; + } + return cwd; +} + +int restore_cwd(struct saved_cwd *cwd) +{ + int rc = chdir(cwd->cwd); + free(cwd); + return rc; +} diff --git a/save-cwd.h b/save-cwd.h new file mode 100644 index 0000000..1087ed3 --- /dev/null +++ b/save-cwd.h @@ -0,0 +1,3 @@ +struct saved_cwd; +struct saved_cwd *save_cwd(void); +int restore_cwd(struct saved_cwd *); diff --git a/unix-socket.c b/unix-socket.c index 01f119f..65d92f2 100644 --- a/unix-socket.c +++ b/unix-socket.c @@ -18,19 +18,19 @@ static int chdir_len(const char *orig, int len) } struct unix_sockaddr_context { - char orig_dir[PATH_MAX]; + struct saved_cwd *orig_dir; }; static void unix_sockaddr_cleanup(struct unix_sockaddr_context *ctx) { - if (!ctx->orig_dir[0]) + if (!ctx->orig_dir) return; /* * If we fail, we can't just return an error, since we have * moved the cwd of the whole process, which could confuse calling * code. We are better off to just die. */ - if (chdir(ctx->orig_dir) < 0) + if (restore_cwd(ctx->orig_dir)) die("unable to restore original working directory"); } @@ -39,7 +39,7 @@ static int unix_sockaddr_init(struct sockaddr_un *sa, const char *path, { int size = strlen(path) + 1; - ctx->orig_dir[0] = '\0'; + ctx->orig_dir = NULL; if (size > sizeof(sa->sun_path)) { const char *slash = find_last_dir_sep(path); const char *dir; @@ -57,7 +57,8 @@ static int unix_sockaddr_init(struct sockaddr_un *sa, const char *path, return -1; } - if (!getcwd(ctx->orig_dir, sizeof(ctx->orig_dir))) { + ctx->orig_dir = save_cwd(); + if (!ctx->orig_dir) { errno = ENAMETOOLONG; return -1; } -- 2.0.2 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html