Re: [PATCH 5/6] src/fssum: Allow single file input

[Arvind Raghavan <raghavan.arvind@gmail.com>]

On 05/21, Amir Goldstein wrote:
> On Thu, May 21, 2020 at 3:10 AM Arvind Raghavan
> <raghavan.arvind@gmail.com> wrote:
> >
> > Allow regular links and symlinks to be passed as input to fssum.
> >
> > 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 | 35 ++++++++++++++++++++++++++++++++++-
> >  1 file changed, 34 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/fssum.c b/src/fssum.c
> > index ece0f556..2d1624ca 100644
> > --- a/src/fssum.c
> > +++ b/src/fssum.c
> > @@ -29,6 +29,7 @@
> >  #include <inttypes.h>
> >  #include <assert.h>
> >  #include <endian.h>
> > +#include <libgen.h>
> >
> >  #define CS_SIZE 16
> >  #define CHUNKS 128
> > @@ -884,8 +885,40 @@ main(int argc, char *argv[])
> >         if (gen_manifest)
> >                 fprintf(out_fp, "Flags: %s\n", flagstring);
> >
> > +       struct stat64 path_st;
> > +       if (fstat64(fd, &path_st)) {
> > +               perror("fstat");
> > +               exit(-1);
> > +       }
> > +
> >         sum_init(&cs);
> > -       sum(fd, 1, &cs, path, "");
> > +
> > +       if (S_ISDIR(path_st.st_mode)) {
> > +               sum(fd, 1, &cs, path, "");
> > +       } else if (S_ISREG(path_st.st_mode) || S_ISLNK(path_st.st_mode)) {
> > +               // Copy because dirname may modify path
> > +               char* path_copy = alloc(strlen(path));
> > +               strcpy(path_copy, path);
> > +
> > +               char* dir_path = dirname(path);
> > +               char* name = basename(path_copy);
> > +
> > +               int dirfd = open(dir_path, O_RDONLY);
> > +               if (fd == -1) {
> > +                       fprintf(stderr, "failed to open %s: %s\n", dir_path,
> > +                               strerror(errno));
> > +                       exit(-1);
> > +               }
> > +
> > +               sum_one(dirfd, 1, &cs, dir_path, "", name);
> 
> Instead of all of the above, how about just:
>                sum_one(fd, 1, &cs, path, "", "");
> 
> From looking at sum_one() code, it seems to me like that will work,
> but I may be missing something.
> It's not that you *want* the name in the checksum, it is not even
> part of the metadata that is being synced with fsync.

The issue here is that we preserved the code from sum which does
all its opens using openat with the parent directory fd and a
filename. Since we're trying to reuse that code I believe we need
to have this somewhat ugly boilerplate.

> Other than that patch set looks excellent.
> Very pleasant for review :-)

Thanks! :)

> One little thing is missing from the cover letter -
> Which tests did you run to verify these changes do not regress existing
> tests?

I just ran the relevant tests and encountered a small issue with
the refactoring patch. This is my bad, since we changed lstat to
use fstatat, we are no longer doing a fchdir which a readlink
call later on relies on. I can fix it by changing the readlink to
a readlinkat.

I'll add that change and add the set of relevant patches to the
cover letter in a V2.

> Feel free to add to all the other patches in the series:
> Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> 
> If you are able to change sum_one() of empty name as I suggested,
> feel free to add that to this patch as well.
> 
> Thanks,
> Amir.