> > > This helper seems overly complicated for the use case in this > > > test, but it is local to the test. I understand it is auto generated for > > > other tests but that's not the way to go. > > > I suggest you look into using existing FSSUM_PROG tool. > > > > I agree that seems like a more elegant solution. We essentially > > have four cases that we need to check: > > > > (1) fsync file (meta + data) > > (2) fdatasync file > > (3) fsync dir > > (4) fdatasync dir > > > > FSSUM_PROG works great for (3) and (4), since it looks like its > > intended to be used only on directories. md5sum works well for > > (2), but it doesn't capture the metadata needed for (1). > > FSSUM_PROG has a way to print a full "manifest", which contains > > the metadata/data hashes for each file, so I believe we can > > grep/cut it out of the manifest output to test for (1). I'll send > > out a V2 patch if you think this seems like a reasonable > > approach. > > > > Also, see below for a note on FSSUM_PROG. > > Upon looking more closely into fssum, I've changed my mind. It > looks like fssum walks a directory recursively, adding all file > names, metadata, and data into a checksum. This is useful for a > filesytem with stronger guarantees, but under POSIX standards, > the only guarantees for fsycing a dir are that its immediate > dirents are correct and that its metadata is synced, not > necessarily the metadata of its children. > > Because of this, I believe it would be more readable and concise > to rewrite this function instead of grep/cutting the output of > fssum. I can clean it up by removing the echos and case/shifting > and turn it into something that concisely expresses the > following: > > fsync file -> stat file, md5sum file > fdatasync file -> md5sum file > fsync dir -> stat dir, ls dir > fdatasync dir -> ls dir > > Would those changes make more sense? > Either this or add support for single file and non recursive mode to fssum. Either would be fine by me, but latter would make your tests overall better. [...] > > > > I believe there is a minor bug in the fssum program. Below are > > the relevant lines from the help output. > > > > usage: fssum <options> <path> > > options: > > ... > > -[ugoamcdxe]: specify which fields to include in checksum > > calculation. > > ... > > x : include xattrs > > ... > > ... > > -x path : exclude path when building checksum > > (multiple ok) > > ... > > > > There seems to be a duplicate flag which prevents the user from > > passing in -x to include the xattrs field in checksum > > calculation. This can be fixed with a simple patch that changes > > the latter flag to "-i path", and updates the documentation to > > say "ignores path when building checksum". I've got such a commit > > built locally than I can send out in a patch if you think it > > makes sense. None of the tests in the repo use the flag as AFAIK, > > but we will end up using it for the xattr tests. Sure. please send the patch. Also need to fix the documentation of default flags: "...The default field mask is ugoamCdES" it omits x. Thanks, Amir.
