On Tue, May 19, 2020 at 1:30 AM Arvind Raghavan <raghavan.arvind@xxxxxxxxx> 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@xxxxxxxxx> > Signed-off-by: Jayashree Mohan <jaya@xxxxxxxxxxxxx> > Signed-off-by: Vijay Chidambaram <vijay@xxxxxxxxxxxxx> > --- > 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.