* [PATCH] src/fssum: Refactor recursive traversal
@ 2020-05-18 20:15 Arvind Raghavan
2020-05-19 4:58 ` Amir Goldstein
0 siblings, 1 reply; 2+ messages in thread
From: Arvind Raghavan @ 2020-05-18 20:15 UTC (permalink / raw)
To: fstests, Amir Goldstein
Cc: Arvind Raghavan, Jayashree Mohan, Vijay Chidambaram
Moves some logic from the recursive directory traversal into a helper
function to make it easier to add support for regular files. Does not
change functionality.
Signed-off-by: Arvind Raghavan <raghavan.arvind@gmail.com>
Signed-off-by: Jayashree Mohan <jaya@cs.utexas.edu>
Signed-off-by: Vijay Chidambaram <vijay@cs.utexas.edu>
---
src/fssum.c | 298 ++++++++++++++++++++++++++++------------------------
1 file changed, 162 insertions(+), 136 deletions(-)
diff --git a/src/fssum.c b/src/fssum.c
index 3d97a70b..f2325ae0 100644
--- a/src/fssum.c
+++ b/src/fssum.c
@@ -502,6 +502,162 @@ malformed:
excess_file(fn);
}
+void
+sum(int dirfd, int level, sum_t *dircs, char *path_prefix, char *path_in);
+
+void
+sum_one(int dirfd, int level, sum_t *dircs, char *path_prefix,
+ char *path_in, char *name) {
+ sum_t cs;
+ sum_t meta;
+ int fd;
+ int ret;
+ int excl;
+ char* path;
+ struct stat64 st;
+ sum_file_data_t sum_file_data = flags[FLAG_STRUCTURE] ?
+ sum_file_data_strict : sum_file_data_permissive;
+
+ sum_init(&cs);
+ sum_init(&meta);
+ path = alloc(strlen(path_in) + strlen(name) + 3);
+ sprintf(path, "%s/%s", path_in, name);
+ for (excl = 0; excl < n_excludes; ++excl) {
+ if (strncmp(excludes[excl].path, path,
+ excludes[excl].len) == 0)
+ goto out;
+ }
+
+ ret = fchdir(dirfd);
+ if (ret == -1) {
+ perror("fchdir");
+ exit(-1);
+ }
+ ret = lstat64(name, &st);
+ if (ret) {
+ fprintf(stderr, "stat failed for %s/%s: %s\n",
+ path_prefix, path, strerror(errno));
+ exit(-1);
+ }
+
+ sum_add_u64(&meta, level);
+ sum_add(&meta, name, strlen(name));
+ if (!S_ISDIR(st.st_mode))
+ sum_add_u64(&meta, st.st_nlink);
+ if (flags[FLAG_UID])
+ sum_add_u64(&meta, st.st_uid);
+ if (flags[FLAG_GID])
+ sum_add_u64(&meta, st.st_gid);
+ if (flags[FLAG_MODE])
+ sum_add_u64(&meta, st.st_mode);
+ if (flags[FLAG_ATIME])
+ sum_add_time(&meta, st.st_atime);
+ if (flags[FLAG_MTIME])
+ sum_add_time(&meta, st.st_mtime);
+ if (flags[FLAG_CTIME])
+ sum_add_time(&meta, st.st_ctime);
+ if (flags[FLAG_XATTRS] &&
+ (S_ISDIR(st.st_mode) || S_ISREG(st.st_mode))) {
+ fd = openat(dirfd, name, 0);
+ if (fd == -1 && flags[FLAG_OPEN_ERROR]) {
+ sum_add_u64(&meta, errno);
+ } else if (fd == -1) {
+ fprintf(stderr, "open failed for %s/%s: %s\n",
+ path_prefix, path, strerror(errno));
+ exit(-1);
+ } else {
+ ret = sum_xattrs(fd, &meta);
+ close(fd);
+ if (ret < 0) {
+ fprintf(stderr,
+ "failed to read xattrs from "
+ "%s/%s: %s\n",
+ path_prefix, path,
+ strerror(-ret));
+ exit(-1);
+ }
+ }
+ }
+ if (S_ISDIR(st.st_mode)) {
+ fd = openat(dirfd, name, 0);
+ if (fd == -1 && flags[FLAG_OPEN_ERROR]) {
+ sum_add_u64(&meta, errno);
+ } else if (fd == -1) {
+ fprintf(stderr, "open failed for %s/%s: %s\n",
+ path_prefix, path, strerror(errno));
+ exit(-1);
+ } else {
+ sum(fd, level + 1, &cs, path_prefix, path);
+ close(fd);
+ }
+ } else if (S_ISREG(st.st_mode)) {
+ sum_add_u64(&meta, st.st_size);
+ if (flags[FLAG_DATA]) {
+ if (verbose)
+ fprintf(stderr, "file %s\n",
+ name);
+ fd = openat(dirfd, name, 0);
+ if (fd == -1 && flags[FLAG_OPEN_ERROR]) {
+ sum_add_u64(&meta, errno);
+ } else if (fd == -1) {
+ fprintf(stderr,
+ "open failed for %s/%s: %s\n",
+ path_prefix, path,
+ strerror(errno));
+ exit(-1);
+ }
+ if (fd != -1) {
+ ret = sum_file_data(fd, &cs);
+ if (ret < 0) {
+ fprintf(stderr,
+ "read failed for "
+ "%s/%s: %s\n",
+ path_prefix, path,
+ strerror(errno));
+ exit(-1);
+ }
+ close(fd);
+ }
+ }
+ } else if (S_ISLNK(st.st_mode)) {
+ ret = readlink(name, buf, sizeof(buf));
+ if (ret == -1) {
+ perror("readlink");
+ exit(-1);
+ }
+ sum_add(&cs, buf, ret);
+ } else if (S_ISCHR(st.st_mode) || S_ISBLK(st.st_mode)) {
+ sum_add_u64(&cs, major(st.st_rdev));
+ sum_add_u64(&cs, minor(st.st_rdev));
+ }
+ sum_fini(&cs);
+ sum_fini(&meta);
+ if (gen_manifest || in_manifest) {
+ char *fn;
+ char *m;
+ char *c;
+
+ if (S_ISDIR(st.st_mode))
+ strcat(path, "/");
+ fn = escape(path);
+ m = sum_to_string(&meta);
+ c = sum_to_string(&cs);
+
+ if (gen_manifest)
+ fprintf(out_fp, "%s %s %s\n", fn, m, c);
+ if (in_manifest)
+ check_manifest(fn, m, c, 0);
+ free(c);
+ free(m);
+ free(fn);
+ }
+ sum_add_sum(dircs, &cs);
+ sum_add_sum(dircs, &meta);
+
+out:
+ free(path);
+}
+
void
sum(int dirfd, int level, sum_t *dircs, char *path_prefix, char *path_in)
{
@@ -512,10 +668,6 @@ sum(int dirfd, int level, sum_t *dircs, char *path_prefix, char *path_in)
int entries = 0;
int i;
int ret;
- int fd;
- int excl;
- sum_file_data_t sum_file_data = flags[FLAG_STRUCTURE] ?
- sum_file_data_strict : sum_file_data_permissive;
struct stat64 dir_st;
if (fstat64(dirfd, &dir_st)) {
@@ -550,19 +702,6 @@ sum(int dirfd, int level, sum_t *dircs, char *path_prefix, char *path_in)
qsort(namelist, entries, sizeof(*namelist), namecmp);
for (i = 0; i < entries; ++i) {
struct stat64 st;
- sum_t cs;
- sum_t meta;
- char *path;
-
- sum_init(&cs);
- sum_init(&meta);
- path = alloc(strlen(path_in) + strlen(namelist[i]) + 3);
- sprintf(path, "%s/%s", path_in, namelist[i]);
- for (excl = 0; excl < n_excludes; ++excl) {
- if (strncmp(excludes[excl].path, path,
- excludes[excl].len) == 0)
- goto next;
- }
ret = fchdir(dirfd);
if (ret == -1) {
@@ -571,130 +710,17 @@ sum(int dirfd, int level, sum_t *dircs, char *path_prefix, char *path_in)
}
ret = lstat64(namelist[i], &st);
if (ret) {
- fprintf(stderr, "stat failed for %s/%s: %s\n",
- path_prefix, path, strerror(errno));
+ fprintf(stderr, "stat failed for %s/%s/%s: %s\n",
+ path_prefix, path_in, namelist[i],
+ strerror(errno));
exit(-1);
}
/* We are crossing into a different subvol, skip this subtree. */
if (st.st_dev != dir_st.st_dev)
- goto next;
-
- sum_add_u64(&meta, level);
- sum_add(&meta, namelist[i], strlen(namelist[i]));
- if (!S_ISDIR(st.st_mode))
- sum_add_u64(&meta, st.st_nlink);
- if (flags[FLAG_UID])
- sum_add_u64(&meta, st.st_uid);
- if (flags[FLAG_GID])
- sum_add_u64(&meta, st.st_gid);
- if (flags[FLAG_MODE])
- sum_add_u64(&meta, st.st_mode);
- if (flags[FLAG_ATIME])
- sum_add_time(&meta, st.st_atime);
- if (flags[FLAG_MTIME])
- sum_add_time(&meta, st.st_mtime);
- if (flags[FLAG_CTIME])
- sum_add_time(&meta, st.st_ctime);
- if (flags[FLAG_XATTRS] &&
- (S_ISDIR(st.st_mode) || S_ISREG(st.st_mode))) {
- fd = openat(dirfd, namelist[i], 0);
- if (fd == -1 && flags[FLAG_OPEN_ERROR]) {
- sum_add_u64(&meta, errno);
- } else if (fd == -1) {
- fprintf(stderr, "open failed for %s/%s: %s\n",
- path_prefix, path, strerror(errno));
- exit(-1);
- } else {
- ret = sum_xattrs(fd, &meta);
- close(fd);
- if (ret < 0) {
- fprintf(stderr,
- "failed to read xattrs from "
- "%s/%s: %s\n",
- path_prefix, path,
- strerror(-ret));
- exit(-1);
- }
- }
- }
- if (S_ISDIR(st.st_mode)) {
- fd = openat(dirfd, namelist[i], 0);
- if (fd == -1 && flags[FLAG_OPEN_ERROR]) {
- sum_add_u64(&meta, errno);
- } else if (fd == -1) {
- fprintf(stderr, "open failed for %s/%s: %s\n",
- path_prefix, path, strerror(errno));
- exit(-1);
- } else {
- sum(fd, level + 1, &cs, path_prefix, path);
- close(fd);
- }
- } else if (S_ISREG(st.st_mode)) {
- sum_add_u64(&meta, st.st_size);
- if (flags[FLAG_DATA]) {
- if (verbose)
- fprintf(stderr, "file %s\n",
- namelist[i]);
- fd = openat(dirfd, namelist[i], 0);
- if (fd == -1 && flags[FLAG_OPEN_ERROR]) {
- sum_add_u64(&meta, errno);
- } else if (fd == -1) {
- fprintf(stderr,
- "open failed for %s/%s: %s\n",
- path_prefix, path,
- strerror(errno));
- exit(-1);
- }
- if (fd != -1) {
- ret = sum_file_data(fd, &cs);
- if (ret < 0) {
- fprintf(stderr,
- "read failed for "
- "%s/%s: %s\n",
- path_prefix, path,
- strerror(errno));
- exit(-1);
- }
- close(fd);
- }
- }
- } else if (S_ISLNK(st.st_mode)) {
- ret = readlink(namelist[i], buf, sizeof(buf));
- if (ret == -1) {
- perror("readlink");
- exit(-1);
- }
- sum_add(&cs, buf, ret);
- } else if (S_ISCHR(st.st_mode) || S_ISBLK(st.st_mode)) {
- sum_add_u64(&cs, major(st.st_rdev));
- sum_add_u64(&cs, minor(st.st_rdev));
- }
- sum_fini(&cs);
- sum_fini(&meta);
- if (gen_manifest || in_manifest) {
- char *fn;
- char *m;
- char *c;
-
- if (S_ISDIR(st.st_mode))
- strcat(path, "/");
- fn = escape(path);
- m = sum_to_string(&meta);
- c = sum_to_string(&cs);
-
- if (gen_manifest)
- fprintf(out_fp, "%s %s %s\n", fn, m, c);
- if (in_manifest)
- check_manifest(fn, m, c, 0);
- free(c);
- free(m);
- free(fn);
- }
- sum_add_sum(dircs, &cs);
- sum_add_sum(dircs, &meta);
-next:
- free(path);
+ continue;
+
+ sum_one(dirfd, level, dircs, path_prefix, path_in, namelist[i]);
}
}
--
2.20.1
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH] src/fssum: Refactor recursive traversal
2020-05-18 20:15 [PATCH] src/fssum: Refactor recursive traversal Arvind Raghavan
@ 2020-05-19 4:58 ` Amir Goldstein
0 siblings, 0 replies; 2+ messages in thread
From: Amir Goldstein @ 2020-05-19 4:58 UTC (permalink / raw)
To: Arvind Raghavan; +Cc: fstests, Jayashree Mohan, Vijay Chidambaram
On Tue, May 19, 2020 at 1:30 AM Arvind Raghavan
<raghavan.arvind@gmail.com> wrote:
>
> Moves some logic from the recursive directory traversal into a helper
> function to make it easier to add support for regular files. Does not
> change functionality.
>
Arvind,
The main comment is that this patch by itself is not eligible for merging.
It should be part of a patch series.
One more tip for ease of review - don't mix re-factor with moving chunks
of code. I reviewed this patch by moving sum_one() below sum(), where
the chunk of code was before the re-factoring and using diff -w.
Check it out to see how easy it is to review.
There is not really a reason to put the sum_one() helper on top as it
anyway depends on forward declaration of sum(), so it can be the other
way around.
See a couple of minor suggestions below.
Thanks,
Amir.
> Signed-off-by: Arvind Raghavan <raghavan.arvind@gmail.com>
> Signed-off-by: Jayashree Mohan <jaya@cs.utexas.edu>
> Signed-off-by: Vijay Chidambaram <vijay@cs.utexas.edu>
> ---
> src/fssum.c | 298 ++++++++++++++++++++++++++++------------------------
> 1 file changed, 162 insertions(+), 136 deletions(-)
>
> diff --git a/src/fssum.c b/src/fssum.c
> index 3d97a70b..f2325ae0 100644
> --- a/src/fssum.c
> +++ b/src/fssum.c
> @@ -502,6 +502,162 @@ malformed:
> excess_file(fn);
> }
>
> +void
> +sum(int dirfd, int level, sum_t *dircs, char *path_prefix, char *path_in);
> +
> +void
> +sum_one(int dirfd, int level, sum_t *dircs, char *path_prefix,
> + char *path_in, char *name) {
> + sum_t cs;
> + sum_t meta;
> + int fd;
> + int ret;
> + int excl;
> + char* path;
> + struct stat64 st;
> + sum_file_data_t sum_file_data = flags[FLAG_STRUCTURE] ?
> + sum_file_data_strict : sum_file_data_permissive;
It's silly to do that every "one". flags is global and doesn't change,
so sum_file_data may be global as well and set on main.
Do that before refactoring patch.
[...]
>
> ret = fchdir(dirfd);
> if (ret == -1) {
> @@ -571,130 +710,17 @@ sum(int dirfd, int level, sum_t *dircs, char *path_prefix, char *path_in)
> }
> ret = lstat64(namelist[i], &st);
If you change that to fstatat(dirfd, ... AT_SYMLINK_NOFOLLOW),
fchdir() above will not be needed.
that change you can do with the refactoring patch.
Thanks,
Amir.
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2020-05-19 4:58 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-18 20:15 [PATCH] src/fssum: Refactor recursive traversal Arvind Raghavan
2020-05-19 4:58 ` Amir Goldstein
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.