"GOT", but the "O" is a cute, smiling pufferfish. Index | Thread | Search

From:
Yang Zhong <yzhong@freebsdfoundation.org>
Subject:
change got_worktree_init, open_worktree to use fds
To:
gameoftrees@openbsd.org
Date:
Mon, 30 Nov 2020 09:42:51 -0800

Download raw body.

Thread
Here are some simple changes for initializing and opening worktrees.
In addition to open() to openat()-style changes, I've also added
fd fields to the worktree struct, as I'll need it for changes like
this in other parts of the program.

The helper update_meta_file wasn't changed to use fds here, as it
requires a function like mkostempsat().

I've changed the signature of got_worktree_init to take in an fd, as
it's only called in one place. I understand that changing
got_worktree_open's signature should be avoided, so currently I have
got_worktree_open open the fd inside the function instead of taking
it in. I'm not sure what the best solution is in this case.

I've also changed got_worktree_open to no longer call realpath() on
the worktree_path. Instead, where it is needed I make the path
absolute beforehand (It's only needed for checkout). This is more
amenable to Capsicum, and doesn't require much change.

I think these changes are mostly upstream-friendly. I would appreciate
if you could evaluate the direction of these changes.

diff --git a/got/got.c b/got/got.c
index 0357368c..33c8cfa6 100644
--- a/got/got.c
+++ b/got/got.c
@@ -2711,6 +2711,20 @@ cmd_checkout(int argc, char *argv[])
             goto done;
         }
     }
+    int worktree_fd = open(worktree_path, O_DIRECTORY);
+    if (worktree_fd == -1) {
+        error = got_error_from_errno2("open", worktree_path);
+        goto done;
+    }
+
+    /* Make worktree path absolute */
+    char *worktree_path_abs = realpath(worktree_path, NULL);
+    if (worktree_path_abs == NULL) {
+        error = got_error_from_errno2("realpath", worktree_path_abs);
+        goto done;
+    }
+    free(worktree_path);
+    worktree_path = worktree_path_abs;

     error = apply_unveil(got_repo_get_path(repo), 0, worktree_path);
     if (error)
@@ -2720,7 +2734,7 @@ cmd_checkout(int argc, char *argv[])
     if (error != NULL)
         goto done;

-    error = got_worktree_init(worktree_path, head_ref, path_prefix, repo);
+    error = got_worktree_init(worktree_fd, worktree_path, head_ref,
path_prefix, repo);
     if (error != NULL && !(error->code == GOT_ERR_ERRNO && errno == EEXIST))
         goto done;

diff --git a/include/got_path.h b/include/got_path.h
index 0219d3d3..04558384 100644
--- a/include/got_path.h
+++ b/include/got_path.h
@@ -132,3 +132,4 @@ const struct got_error *got_path_find_prog(char
**, const char *);

 /* Create a new file at a specified path, with optional content. */
 const struct got_error *got_path_create_file(const char *, const char *);
+const struct got_error *got_path_create_fileat(int, const char *,
const char *);
diff --git a/include/got_worktree.h b/include/got_worktree.h
index 24fddd52..13b13521 100644
--- a/include/got_worktree.h
+++ b/include/got_worktree.h
@@ -42,12 +42,11 @@ struct got_fileindex;
 /*
  * Attempt to initialize a new work tree on disk.
  * The first argument is the path to a directory where the work tree
- * will be created. The path itself must not yet exist, but the dirname(3)
- * of the path must already exist.
+ * will be created. The dirname(3) of the path must already exist.
  * The reference provided will be used to determine the new worktree's
  * base commit. The third argument speficies the work tree's path prefix.
  */
-const struct got_error *got_worktree_init(const char *, struct got_reference *,
+const struct got_error *got_worktree_init(int, const char *, struct
got_reference *,
     const char *, struct got_repository *);

 /*
diff --git a/lib/got_lib_worktree.h b/lib/got_lib_worktree.h
index 067bac22..2a1f9582 100644
--- a/lib/got_lib_worktree.h
+++ b/lib/got_lib_worktree.h
@@ -17,6 +17,8 @@
 struct got_worktree {
     char *root_path;
     char *repo_path;
+    int root_fd;
+    int repo_fd;
     char *path_prefix;
     struct got_object_id *base_commit_id;
     char *head_ref_name;
diff --git a/lib/path.c b/lib/path.c
index a670334c..655304a0 100644
--- a/lib/path.c
+++ b/lib/path.c
@@ -513,11 +513,17 @@ got_path_find_prog(char **filename, const char *prog)

 const struct got_error *
 got_path_create_file(const char *path, const char *content)
+{
+    return got_path_create_fileat(AT_FDCWD, path, content);
+}
+
+const struct got_error *
+got_path_create_fileat(int dir_fd, const char *path, const char *content)
 {
     const struct got_error *err = NULL;
     int fd = -1;

-    fd = open(path, O_RDWR | O_CREAT | O_EXCL | O_NOFOLLOW,
+    fd = openat(dir_fd, path, O_RDWR | O_CREAT | O_EXCL | O_NOFOLLOW,
         GOT_DEFAULT_FILE_MODE);
     if (fd == -1) {
         err = got_error_from_errno2("open", path);
diff --git a/lib/worktree.c b/lib/worktree.c
index 2e265b00..30ce5dea 100644
--- a/lib/worktree.c
+++ b/lib/worktree.c
@@ -67,15 +67,15 @@
 #define GOT_MERGE_LABEL_BASE    "3-way merge base"

 static const struct got_error *
-create_meta_file(const char *path_got, const char *name, const char *content)
+create_meta_file(int worktree_fd, const char *name, const char *content)
 {
     const struct got_error *err = NULL;
     char *path;

-    if (asprintf(&path, "%s/%s", path_got, name) == -1)
+    if (asprintf(&path, "%s/%s", GOT_WORKTREE_GOT_DIR, name) == -1)
         return got_error_from_errno("asprintf");

-    err = got_path_create_file(path, content);
+    err = got_path_create_fileat(worktree_fd, path, content);
     free(path);
     return err;
 }
@@ -120,7 +120,7 @@ done:
 }

 static const struct got_error *
-read_meta_file(char **content, const char *path_got, const char *name)
+read_meta_file(char **content, int worktree_fd, const char *path_got,
const char *name)
 {
     const struct got_error *err = NULL;
     char *path;
@@ -136,7 +136,7 @@ read_meta_file(char **content, const char
*path_got, const char *name)
         goto done;
     }

-    fd = open(path, O_RDONLY | O_NOFOLLOW);
+    fd = openat(worktree_fd, path, O_RDONLY | O_NOFOLLOW);
     if (fd == -1) {
         if (errno == ENOENT)
             err = got_error_path(path, GOT_ERR_WORKTREE_META);
@@ -204,7 +204,7 @@ write_head_ref(const char *path_got, struct
got_reference *head_ref)
 }

 const struct got_error *
-got_worktree_init(const char *path, struct got_reference *head_ref,
+got_worktree_init(int fd, const char *path, struct got_reference *head_ref,
     const char *prefix, struct got_repository *repo)
 {
     const struct got_error *err = NULL;
@@ -237,29 +237,23 @@ got_worktree_init(const char *path, struct
got_reference *head_ref,
             return got_error_from_errno("asprintf");
     }

-    /* Create top-level directory (may already exist). */
-    if (mkdir(path, GOT_DEFAULT_DIR_MODE) == -1 && errno != EEXIST) {
-        err = got_error_from_errno2("mkdir", path);
-        goto done;
-    }
-
     /* Create .got directory (may already exist). */
     if (asprintf(&path_got, "%s/%s", path, GOT_WORKTREE_GOT_DIR) == -1) {
         err = got_error_from_errno("asprintf");
         goto done;
     }
-    if (mkdir(path_got, GOT_DEFAULT_DIR_MODE) == -1 && errno != EEXIST) {
+    if (mkdirat(fd, GOT_WORKTREE_GOT_DIR, GOT_DEFAULT_DIR_MODE) == -1
&& errno != EEXIST) {
         err = got_error_from_errno2("mkdir", path_got);
         goto done;
     }

     /* Create an empty lock file. */
-    err = create_meta_file(path_got, GOT_WORKTREE_LOCK, NULL);
+    err = create_meta_file(fd, GOT_WORKTREE_LOCK, NULL);
     if (err)
         goto done;

     /* Create an empty file index. */
-    err = create_meta_file(path_got, GOT_WORKTREE_FILE_INDEX, NULL);
+    err = create_meta_file(fd, GOT_WORKTREE_FILE_INDEX, NULL);
     if (err)
         goto done;

@@ -272,18 +266,18 @@ got_worktree_init(const char *path, struct
got_reference *head_ref,
     err = got_object_id_str(&basestr, commit_id);
     if (err)
         goto done;
-    err = create_meta_file(path_got, GOT_WORKTREE_BASE_COMMIT, basestr);
+    err = create_meta_file(fd, GOT_WORKTREE_BASE_COMMIT, basestr);
     if (err)
         goto done;

     /* Store path to repository. */
-    err = create_meta_file(path_got, GOT_WORKTREE_REPOSITORY,
+    err = create_meta_file(fd, GOT_WORKTREE_REPOSITORY,
         got_repo_get_path(repo));
     if (err)
         goto done;

     /* Store in-repository path prefix. */
-    err = create_meta_file(path_got, GOT_WORKTREE_PATH_PREFIX,
+    err = create_meta_file(fd, GOT_WORKTREE_PATH_PREFIX,
         absprefix ? absprefix : prefix);
     if (err)
         goto done;
@@ -299,7 +293,7 @@ got_worktree_init(const char *path, struct
got_reference *head_ref,
         err = got_error_uuid(uuid_status, "uuid_to_string");
         goto done;
     }
-    err = create_meta_file(path_got, GOT_WORKTREE_UUID, uuidstr);
+    err = create_meta_file(fd, GOT_WORKTREE_UUID, uuidstr);
     if (err)
         goto done;

@@ -308,7 +302,7 @@ got_worktree_init(const char *path, struct
got_reference *head_ref,
         err = got_error_from_errno("asprintf");
         goto done;
     }
-    err = create_meta_file(path_got, GOT_WORKTREE_FORMAT, formatstr);
+    err = create_meta_file(fd, GOT_WORKTREE_FORMAT, formatstr);
     if (err)
         goto done;

@@ -323,10 +317,9 @@ done:
 }

 static const struct got_error *
-open_worktree(struct got_worktree **worktree, const char *path)
+open_worktree(struct got_worktree **worktree, int worktree_fd, const
char *path)
 {
     const struct got_error *err = NULL;
-    char *path_got;
     char *formatstr = NULL;
     char *uuidstr = NULL;
     char *path_lock = NULL;
@@ -338,26 +331,20 @@ open_worktree(struct got_worktree **worktree,
const char *path)

     *worktree = NULL;

-    if (asprintf(&path_got, "%s/%s", path, GOT_WORKTREE_GOT_DIR) == -1) {
-        err = got_error_from_errno("asprintf");
-        path_got = NULL;
-        goto done;
-    }
-
-    if (asprintf(&path_lock, "%s/%s", path_got, GOT_WORKTREE_LOCK) == -1) {
+    if (asprintf(&path_lock, "%s/%s", GOT_WORKTREE_GOT_DIR,
GOT_WORKTREE_LOCK) == -1) {
         err = got_error_from_errno("asprintf");
         path_lock = NULL;
         goto done;
     }

-    fd = open(path_lock, O_RDWR | O_EXLOCK | O_NONBLOCK);
+    fd = openat(worktree_fd, path_lock, O_RDWR | O_EXLOCK | O_NONBLOCK);
     if (fd == -1) {
         err = (errno == EWOULDBLOCK ? got_error(GOT_ERR_WORKTREE_BUSY)
             : got_error_from_errno2("open", path_lock));
         goto done;
     }

-    err = read_meta_file(&formatstr, path_got, GOT_WORKTREE_FORMAT);
+    err = read_meta_file(&formatstr, worktree_fd,
GOT_WORKTREE_GOT_DIR, GOT_WORKTREE_FORMAT);
     if (err)
         goto done;

@@ -379,27 +366,27 @@ open_worktree(struct got_worktree **worktree,
const char *path)
     }
     (*worktree)->lockfd = -1;

-    (*worktree)->root_path = realpath(path, NULL);
+    (*worktree)->root_path = strdup(path);
     if ((*worktree)->root_path == NULL) {
-        err = got_error_from_errno2("realpath", path);
+        err = got_error_from_errno2("strdup", path);
         goto done;
     }
-    err = read_meta_file(&(*worktree)->repo_path, path_got,
+    err = read_meta_file(&(*worktree)->repo_path, worktree_fd,
GOT_WORKTREE_GOT_DIR,
         GOT_WORKTREE_REPOSITORY);
     if (err)
         goto done;

-    err = read_meta_file(&(*worktree)->path_prefix, path_got,
+    err = read_meta_file(&(*worktree)->path_prefix, worktree_fd,
GOT_WORKTREE_GOT_DIR,
         GOT_WORKTREE_PATH_PREFIX);
     if (err)
         goto done;

-    err = read_meta_file(&base_commit_id_str, path_got,
+    err = read_meta_file(&base_commit_id_str, worktree_fd,
GOT_WORKTREE_GOT_DIR,
         GOT_WORKTREE_BASE_COMMIT);
     if (err)
         goto done;

-    err = read_meta_file(&uuidstr, path_got, GOT_WORKTREE_UUID);
+    err = read_meta_file(&uuidstr, worktree_fd, GOT_WORKTREE_GOT_DIR,
GOT_WORKTREE_UUID);
     if (err)
         goto done;
     uuid_from_string(uuidstr, &(*worktree)->uuid, &uuid_status);
@@ -417,7 +404,7 @@ open_worktree(struct got_worktree **worktree,
const char *path)
     if (err)
         goto done;

-    err = read_meta_file(&(*worktree)->head_ref_name, path_got,
+    err = read_meta_file(&(*worktree)->head_ref_name, worktree_fd,
GOT_WORKTREE_GOT_DIR,
         GOT_WORKTREE_HEAD_REF);
     if (err)
         goto done;
@@ -431,10 +418,14 @@ open_worktree(struct got_worktree **worktree,
const char *path)

     err = got_gotconfig_read(&(*worktree)->gotconfig,
         (*worktree)->gotconfig_path);
+
+    int repo_fd = open(got_repo_get_path_git_dir(repo), O_DIRECTORY);
+
+    (*worktree)->root_fd = worktree_fd;
+    (*worktree)->repo_fd = repo_fd;
 done:
     if (repo)
         got_repo_close(repo);
-    free(path_got);
     free(path_lock);
     free(base_commit_id_str);
     free(uuidstr);
@@ -456,15 +447,20 @@ got_worktree_open(struct got_worktree
**worktree, const char *path)
 {
     const struct got_error *err = NULL;
     char *worktree_path;
+    int worktree_fd;

     worktree_path = strdup(path);
     if (worktree_path == NULL)
         return got_error_from_errno("strdup");

+    worktree_fd = open(worktree_path, O_DIRECTORY);
+    if (worktree_fd == -1)
+        return got_error_from_errno2("open", worktree_path);
+
     for (;;) {
         char *parent_path;

-        err = open_worktree(worktree, worktree_path);
+        err = open_worktree(worktree, worktree_fd, worktree_path);
         if (err && !(err->code == GOT_ERR_ERRNO && errno == ENOENT)) {
             free(worktree_path);
             return err;
@@ -484,7 +480,9 @@ got_worktree_open(struct got_worktree **worktree,
const char *path)
             break;
         }
         free(worktree_path);
+        close(worktree_fd);
         worktree_path = parent_path;
+        worktree_fd = open(parent_path, O_DIRECTORY);
     }

     free(worktree_path);
@@ -507,6 +505,8 @@ got_worktree_close(struct got_worktree *worktree)
     free(worktree->gotconfig_path);
     got_gotconfig_free(worktree->gotconfig);
     free(worktree);
+    close(worktree->root_fd);
+    close(worktree->repo_fd);
     return err;
 }