Download raw body.
change got_worktree_init, open_worktree to use fds
On Mon, Nov 30, 2020 at 09:42:51AM -0800, Yang Zhong wrote:
> 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.
Thanks!
The patch as received on the mailing list cannot be applied.
Looks like your mailer wrapped some overlong lines:
Patching file got/got.c using Plan A...
Hunk #1 failed at 2711.
patch: **** malformed patch at line 54: path_prefix, repo);
After addressing the review comments below, could you send an updated patch?
And if your mailer cannot send inline patches without mangling them, could
you send the patch as an attachment?
> The helper update_meta_file wasn't changed to use fds here, as it
> requires a function like mkostempsat().
Right. millert@ is already looking into that.
> 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.
So you'll eventually want to pass an fd into got_worktree_open(), right?
Since you're adding 'int root_fd' to struct got_worktree I don't see a
problem with adding a corresponding argument to got_worktree_open() in
order to initialize the root_fd struct member. Upstream code can store an
open directory file descriptor in struct got_worktree and close it in
got_worktree_close(), even if this file descriptor is not otherwise used
directly.
> 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.
Careful, the realpath() call in open_worktree() was added to fix a bug.
Is the test which was added in 7d61d89137e4704e685dacfcfb71a42a87ba9f78
still passing?
Are all the tests passing, for that matter? :)
It would be good to run regression tests on both FreeBSD and OpenBSD
platforms for such patches. As things stand now, the Got 'main' branch
should compile without any extra dependencies in a stock install of
OpenBSD 6.8. How to compile Got and run tests on OpenBSD is documented
in Got's README file.
> I think these changes are mostly upstream-friendly. I would appreciate
> if you could evaluate the direction of these changes.
Please avoid declaring new variables in the middle of a scope. This is
incompatible with OpenBSD style(9) rules, which we try to adhere to.
More feedback inline below:
> 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);
So, for example, 'worktree_fd' should be declared at the beginning of
cmd_checkout().
> + 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);
This should also be declared at the beginning of the scope. You could name
this variable 'worktree_abspath' for consistency with similar variables
elsewhere in the code base. (There's no right or wrong naming here, it's
just that many variables already use the 'abspath' idiom.)
> + 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;
Why is 'repo_fd' stored here?
Shouldn't the repository's directory fd be part of struct got_repository?
> 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;
> - }
Is the above really not needed anymore? As long as the removal of this code
doesn't make any tests fail, removing the code should be fine.
> -
> /* 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);
Another variable declaration which should be moved to the beginning
of this scope.
Everything else about this patch is looking great, thank you!
> +
> + (*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;
> }
>
>
change got_worktree_init, open_worktree to use fds