Download raw body.
capsicum work: add fd field to got_repository, change got_packidx
capsicum work: add fd field to got_repository, change got_packidx
capsicum work: add fd field to got_repository, change got_packidx
On Mon, Dec 14, 2020 at 11:15 AM Stefan Sperling <stsp@stsp.name> wrote:
> > @@ -944,16 +966,20 @@ got_repo_search_packidx(struct got_packidx **packidx, int *idx,
> > }
> > /* No luck. Search the filesystem. */
> >
> > - path_packdir = got_repo_get_path_objects_pack(repo);
> > - if (path_packdir == NULL)
> > - return got_error_from_errno("got_repo_get_path_objects_pack");
> > -
> > - packdir = opendir(path_packdir);
> > - if (packdir == NULL) {
> > + packdir_fd = openat(got_repo_get_path_git_dir_fd(repo),
> > + GOT_OBJECTS_PACK_DIR, O_DIRECTORY);
> > + if (packdir_fd == -1) {
> > if (errno == ENOENT)
> > err = got_error_no_obj(id);
> > else
> > - err = got_error_from_errno2("opendir", path_packdir);
> > + err = got_error_from_errno2("openat",
> > + GOT_OBJECTS_PACK_DIR);
>
> It's slightly sad that we're losing precision in the error message here,
> because the path reported in the error message is no longer absolute.
> Couldn't we keep path_packdir around for this purpose, even if it's just
> used to generate an error message?
It worries me as well, as I've gotten burned by this a few times already.
It's very confusing. I've attached this patch with a possible fix: I made
a variation of got_error_from_errno2 that takes in three parameters,
assumes the last two are path components, and joins them together.
That way, if I have a path relative to, for example, got_repository's root
directory, I can just pass in the repo's root path as well as the relative
one. It's convenient since the repo's fd and path are stored in the same
place. I hope to have relative paths be relative to these root fds
whenever possible, so I feel that this solution will work most of the
time. What do you think?
commit 35ee8c6c6c9ef8da0916e73a55561f040b349bad
Author: Yang Zhong <yzhong@freebsdfoundation.org>
Date: Thu Dec 3 10:19:18 2020 -0800
add fd field to got_repository, alter got_packidx
diff --git a/include/got_error.h b/include/got_error.h
index 5fa8ffa0..21f713d5 100644
--- a/include/got_error.h
+++ b/include/got_error.h
@@ -327,6 +327,15 @@ const struct got_error *got_error_from_errno(const char *);
*/
const struct got_error *got_error_from_errno2(const char *, const char *);
+/*
+ * Get a statically allocated error object with code GOT_ERR_ERRNO
+ * and an error message obtained from strerror(3), prefixed with two
+ * strings. The second and third arguments should be two parts of
+ * a path, and the second prefix is created by joining them.
+ */
+const struct got_error *got_error_from_errno2_join(const char *, const char *,
+ const char *);
+
/*
* Get a statically allocated error object with code GOT_ERR_ERRNO
* and an error message obtained from strerror(3), prefixed with three
diff --git a/include/got_repository.h b/include/got_repository.h
index 9e816e24..58d890d3 100644
--- a/include/got_repository.h
+++ b/include/got_repository.h
@@ -32,6 +32,9 @@ const char *got_repo_get_path(struct got_repository *);
*/
const char *got_repo_get_path_git_dir(struct got_repository *);
+/* Obtain the file descriptor of the repository's .git directory. */
+int got_repo_get_fd(struct got_repository *);
+
/* Obtain the commit author name if parsed from gitconfig, else NULL. */
const char *got_repo_get_gitconfig_author_name(struct got_repository *);
diff --git a/lib/error.c b/lib/error.c
index 055f2bd9..aa532a1d 100644
--- a/lib/error.c
+++ b/lib/error.c
@@ -112,6 +112,21 @@ got_error_from_errno3(const char *prefix, const char *prefix2,
return &err;
}
+const struct got_error *
+got_error_from_errno2_join(const char *prefix, const char *dirpath,
+ const char *relpath)
+{
+ static struct got_error err;
+ static char err_msg[(PATH_MAX * 3) + 20];
+
+ snprintf(err_msg, sizeof(err_msg), "%s: %s/%s: %s", prefix, dirpath,
+ relpath, strerror(errno));
+
+ err.code = GOT_ERR_ERRNO;
+ err.msg = err_msg;
+ return &err;
+}
+
const struct got_error *
got_error_set_errno(int code, const char *prefix)
{
diff --git a/lib/got_lib_pack.h b/lib/got_lib_pack.h
index c56e16c4..39aa4333 100644
--- a/lib/got_lib_pack.h
+++ b/lib/got_lib_pack.h
@@ -168,7 +168,7 @@ struct got_packfile_obj_data {
const struct got_error *got_packidx_init_hdr(struct got_packidx *, int);
const struct got_error *got_packidx_open(struct got_packidx **,
- const char *, int);
+ int, const char *, int);
const struct got_error *got_packidx_close(struct got_packidx *);
int got_packidx_get_object_idx(struct got_packidx *, struct got_object_id *);
const struct got_error *got_packidx_match_id_str_prefix(
diff --git a/lib/got_lib_repository.h b/lib/got_lib_repository.h
index d2a6b78c..18d2953b 100644
--- a/lib/got_lib_repository.h
+++ b/lib/got_lib_repository.h
@@ -34,6 +34,7 @@
struct got_repository {
char *path;
char *path_git_dir;
+ int gitdir_fd;
/* The pack index cache speeds up search for packed objects. */
struct got_packidx *packidx_cache[GOT_PACKIDX_CACHE_SIZE];
diff --git a/lib/pack.c b/lib/pack.c
index 3ce002a8..ea33c2a0 100644
--- a/lib/pack.c
+++ b/lib/pack.c
@@ -330,7 +330,8 @@ done:
}
const struct got_error *
-got_packidx_open(struct got_packidx **packidx, const char *path, int verify)
+got_packidx_open(struct got_packidx **packidx,
+ int packdir_fd, const char *relpath, int verify)
{
const struct got_error *err = NULL;
struct got_packidx *p;
@@ -342,15 +343,15 @@ got_packidx_open(struct got_packidx **packidx, const char *path, int verify)
if (p == NULL)
return got_error_from_errno("calloc");
- p->fd = open(path, O_RDONLY | O_NOFOLLOW);
+ p->fd = openat(packdir_fd, relpath, O_RDONLY | O_NOFOLLOW);
if (p->fd == -1) {
- err = got_error_from_errno2("open", path);
+ err = got_error_from_errno2("openat", relpath);
free(p);
return err;
}
if (fstat(p->fd, &sb) != 0) {
- err = got_error_from_errno2("fstat", path);
+ err = got_error_from_errno2("fstat", relpath);
close(p->fd);
free(p);
return err;
@@ -363,7 +364,7 @@ got_packidx_open(struct got_packidx **packidx, const char *path, int verify)
return err;
}
- p->path_packidx = strdup(path);
+ p->path_packidx = strdup(relpath);
if (p->path_packidx == NULL) {
err = got_error_from_errno("strdup");
goto done;
diff --git a/lib/repository.c b/lib/repository.c
index 02895008..c412ce2f 100644
--- a/lib/repository.c
+++ b/lib/repository.c
@@ -79,6 +79,12 @@ got_repo_get_path_git_dir(struct got_repository *repo)
return repo->path_git_dir;
}
+int
+got_repo_get_fd(struct got_repository *repo)
+{
+ return repo->gitdir_fd;
+}
+
const char *
got_repo_get_gitconfig_author_name(struct got_repository *repo)
{
@@ -335,6 +341,8 @@ open_repo(struct got_repository *repo, const char *path)
{
const struct got_error *err = NULL;
+ repo->gitdir_fd = -1;
+
/* bare git repository? */
repo->path_git_dir = strdup(path);
if (repo->path_git_dir == NULL)
@@ -345,6 +353,12 @@ open_repo(struct got_repository *repo, const char *path)
err = got_error_from_errno("strdup");
goto done;
}
+ repo->gitdir_fd = open(repo->path_git_dir, O_DIRECTORY);
+ if (repo->gitdir_fd == -1) {
+ err = got_error_from_errno2("open",
+ repo->path_git_dir);
+ goto done;
+ }
return NULL;
}
@@ -361,6 +375,12 @@ open_repo(struct got_repository *repo, const char *path)
err = got_error_from_errno("strdup");
goto done;
}
+ repo->gitdir_fd = open(repo->path_git_dir, O_DIRECTORY);
+ if (repo->gitdir_fd == -1) {
+ err = got_error_from_errno2("open",
+ repo->path_git_dir);
+ goto done;
+ }
return NULL;
}
@@ -371,6 +391,10 @@ done:
repo->path = NULL;
free(repo->path_git_dir);
repo->path_git_dir = NULL;
+ if (repo->gitdir_fd != -1)
+ close(repo->gitdir_fd);
+ repo->gitdir_fd = -1;
+
}
return err;
}
@@ -918,11 +942,11 @@ got_repo_search_packidx(struct got_packidx **packidx, int *idx,
struct got_repository *repo, struct got_object_id *id)
{
const struct got_error *err;
- char *path_packdir;
- DIR *packdir;
+ DIR *packdir = NULL;
struct dirent *dent;
char *path_packidx;
size_t i;
+ int packdir_fd;
/* Search pack index cache. */
for (i = 0; i < nitems(repo->packidx_cache); i++) {
@@ -946,16 +970,21 @@ got_repo_search_packidx(struct got_packidx **packidx, int *idx,
}
/* No luck. Search the filesystem. */
- path_packdir = got_repo_get_path_objects_pack(repo);
- if (path_packdir == NULL)
- return got_error_from_errno("got_repo_get_path_objects_pack");
-
- packdir = opendir(path_packdir);
- if (packdir == NULL) {
+ packdir_fd = openat(got_repo_get_fd(repo),
+ GOT_OBJECTS_PACK_DIR, O_DIRECTORY);
+ if (packdir_fd == -1) {
if (errno == ENOENT)
err = got_error_no_obj(id);
else
- err = got_error_from_errno2("opendir", path_packdir);
+ err = got_error_from_errno2_join("openat",
+ got_repo_get_path_git_dir(repo),
+ GOT_OBJECTS_PACK_DIR);
+ goto done;
+ }
+
+ packdir = fdopendir(packdir_fd);
+ if (packdir == NULL) {
+ err = got_error_from_errno("fdopendir");
goto done;
}
@@ -965,7 +994,7 @@ got_repo_search_packidx(struct got_packidx **packidx, int *idx,
if (!is_packidx_filename(dent->d_name, dent->d_namlen))
continue;
- if (asprintf(&path_packidx, "%s/%s", path_packdir,
+ if (asprintf(&path_packidx, "%s/%s", GOT_OBJECTS_PACK_DIR,
dent->d_name) == -1) {
err = got_error_from_errno("asprintf");
goto done;
@@ -985,7 +1014,8 @@ got_repo_search_packidx(struct got_packidx **packidx, int *idx,
continue; /* already searched */
}
- err = got_packidx_open(packidx, path_packidx, 0);
+ err = got_packidx_open(packidx, got_repo_get_fd(repo),
+ path_packidx, 0);
if (err) {
free(path_packidx);
goto done;
@@ -1005,7 +1035,6 @@ got_repo_search_packidx(struct got_packidx **packidx, int *idx,
err = got_error_no_obj(id);
done:
- free(path_packdir);
if (packdir && closedir(packdir) != 0 && err == NULL)
err = got_error_from_errno("closedir");
return err;
@@ -1034,13 +1063,15 @@ read_packfile_hdr(int fd, struct got_packidx *packidx)
}
static const struct got_error *
-open_packfile(int *fd, const char *path_packfile, struct got_packidx *packidx)
+open_packfile(int *fd, struct got_repository *repo,
+ const char *path_packfile, struct got_packidx *packidx)
{
const struct got_error *err = NULL;
- *fd = open(path_packfile, O_RDONLY | O_NOFOLLOW);
+ *fd = openat(got_repo_get_fd(repo), path_packfile, O_RDONLY | O_NOFOLLOW);
if (*fd == -1)
- return got_error_from_errno2("open", path_packfile);
+ return got_error_from_errno2_join("openat",
+ got_repo_get_path_git_dir(repo), path_packfile);
if (packidx) {
err = read_packfile_hdr(*fd, packidx);
@@ -1090,7 +1121,7 @@ got_repo_cache_pack(struct got_pack **packp, struct got_repository *repo,
goto done;
}
- err = open_packfile(&pack->fd, path_packfile, packidx);
+ err = open_packfile(&pack->fd, repo, path_packfile, packidx);
if (err)
goto done;
@@ -1202,22 +1233,25 @@ match_packed_object(struct got_object_id **unique_id,
struct got_repository *repo, const char *id_str_prefix, int obj_type)
{
const struct got_error *err = NULL;
- char *path_packdir;
- DIR *packdir;
+ DIR *packdir = NULL;
struct dirent *dent;
char *path_packidx;
struct got_object_id_queue matched_ids;
+ int packdir_fd;
SIMPLEQ_INIT(&matched_ids);
- path_packdir = got_repo_get_path_objects_pack(repo);
- if (path_packdir == NULL)
- return got_error_from_errno("got_repo_get_path_objects_pack");
+ packdir_fd = openat(got_repo_get_fd(repo),
+ GOT_OBJECTS_PACK_DIR, O_DIRECTORY);
+ if (packdir_fd == -1) {
+ if (errno != ENOENT)
+ err = got_error_from_errno2("openat", GOT_OBJECTS_PACK_DIR);
+ goto done;
+ }
- packdir = opendir(path_packdir);
+ packdir = fdopendir(packdir_fd);
if (packdir == NULL) {
- if (errno != ENOENT)
- err = got_error_from_errno2("opendir", path_packdir);
+ err = got_error_from_errno("fdopendir");
goto done;
}
@@ -1225,17 +1259,17 @@ match_packed_object(struct got_object_id **unique_id,
struct got_packidx *packidx;
struct got_object_qid *qid;
-
if (!is_packidx_filename(dent->d_name, dent->d_namlen))
continue;
- if (asprintf(&path_packidx, "%s/%s", path_packdir,
+ if (asprintf(&path_packidx, "%s/%s", GOT_OBJECTS_PACK_DIR,
dent->d_name) == -1) {
- err = got_error_from_errno("asprintf");
+ err = got_error_from_errno("strdup");
break;
}
- err = got_packidx_open(&packidx, path_packidx, 0);
+ err = got_packidx_open(&packidx, got_repo_get_fd(repo),
+ path_packidx, 0);
free(path_packidx);
if (err)
break;
@@ -1276,7 +1310,6 @@ match_packed_object(struct got_object_id **unique_id,
}
done:
got_object_id_queue_free(&matched_ids);
- free(path_packdir);
if (packdir && closedir(packdir) != 0 && err == NULL)
err = got_error_from_errno("closedir");
if (err) {
capsicum work: add fd field to got_repository, change got_packidx
capsicum work: add fd field to got_repository, change got_packidx
capsicum work: add fd field to got_repository, change got_packidx