FSTests Archive on lore.kernel.org
 help / color / Atom feed
From: Arvind Raghavan <raghavan.arvind@gmail.com>
To: Amir Goldstein <amir73il@gmail.com>, fstests <fstests@vger.kernel.org>
Cc: Jayashree Mohan <jaya@cs.utexas.edu>,
	Vijay Chidambaram <vijay@cs.utexas.edu>
Subject: Re: [PATCH 5/6] src/fssum: Allow single file input
Date: Thu, 21 May 2020 20:06:52 -0500
Message-ID: <20200522010652.x34k3tx7e47jmzzk@gmail.com> (raw)
In-Reply-To: <CAOQ4uxi5C=XOpTS67hS8YOpb-Fo4f6PA5qsoNiDY_k=58r4qJg@mail.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.

  reply index

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-20 21:16 [PATCH 0/6] Changes to fssum to support POSIX Arvind Raghavan
2020-05-20 21:18 ` [PATCH 1/6] src/fssum: Make sum_file_data global Arvind Raghavan
2020-05-20 21:19 ` [PATCH 2/6] src/fssum: Refactor recursive traversal Arvind Raghavan
2020-05-20 21:19 ` [PATCH 3/6] src/fssum: Add flag -R for non-recursive mode Arvind Raghavan
2020-05-20 21:20 ` [PATCH 4/6] src/fssum: Add a flag for including file size in checksum Arvind Raghavan
2020-05-20 21:21 ` [PATCH 5/6] src/fssum: Allow single file input Arvind Raghavan
2020-05-21  9:18   ` Amir Goldstein
2020-05-22  1:06     ` Arvind Raghavan [this message]
2020-05-22  5:37       ` Amir Goldstein
2020-05-20 21:21 ` [PATCH 6/6] src/fssum: Fix whitespace in usage Arvind Raghavan

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200522010652.x34k3tx7e47jmzzk@gmail.com \
    --to=raghavan.arvind@gmail.com \
    --cc=amir73il@gmail.com \
    --cc=fstests@vger.kernel.org \
    --cc=jaya@cs.utexas.edu \
    --cc=vijay@cs.utexas.edu \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

FSTests Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/fstests/0 fstests/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 fstests fstests/ https://lore.kernel.org/fstests \
		fstests@vger.kernel.org
	public-inbox-index fstests

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.fstests


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git