Re: [PATCH 1/2] xfstests: replace hexdump with od command

[Zorro Lang <zlang@redhat.com>]

On Tue, Mar 22, 2022 at 08:54:05AM -0700, Darrick J. Wong wrote:
> On Tue, Mar 22, 2022 at 08:20:15PM +0800, Zorro Lang wrote:
> > On Tue, Mar 22, 2022 at 04:22:59PM +1100, Dave Chinner wrote:
> > > On Mon, Mar 21, 2022 at 07:03:40PM +0800, Zorro Lang wrote:
> > > > The "od" is one of the most fundamental commands in GNU/Linux and
> > > > most Unix-like systems. So we nearly always can count on it, don't
> > > > need to check if it's installed.
> > > > 
> > > > The "hexdump" isn't such fundamental as "od", some systems don't
> > > > install it by default. And as "od" nearly can replace all functions
> > > > of "hexdump", so let's use an unified command "od" to do the hexdump
> > > > job in fstests cases.
> > > > 
> > > > Signed-off-by: Zorro Lang <zlang@redhat.com>
> > > 
> > > Looks good.
> > > 
> > > Reviewed-by: Dave Chinner <dchinner@redhat.com>
> > > 
> > > > ---
> > > > 
> > > > Hi,
> > > > 
> > > > As Dave and Darrick suggested, I did this change, I've tested most of cases,
> > > > except f2fs/001 and ceph/002, but I think they're good. And I used "od"
> > > > command directly in generic/404 and generic/042 for their special reason.
> > > 
> > > That generic/404 usage is ... strange. Why record md5sums of the
> > > encoded hexdump output of the file when you can just run md5sum on
> > > the file directly and get the same information?  i.e.
> > > 
> > > > diff --git a/tests/generic/404 b/tests/generic/404
> > > > index f1e8b0a8..939692eb 100755
> > > > --- a/tests/generic/404
> > > > +++ b/tests/generic/404
> > > > @@ -110,7 +110,7 @@ for (( block=3; block<=500; block++ )); do
> > > >  	# or blocks are in correct order, this commit:
> > > >  	#   2b3864b32403 ("ext4: do not polute the extents cache while shifting extents")
> > > >  	#
> > > > -	md5=`hexdump -e '16/1 "%_p" "\n"' $testfile | md5sum`
> > > > +	md5=`od -An -c $testfile | md5sum`
> > > >  	printf "#%d %s\n" "$block" "$md5"
> > > 
> > > Why isn't this just:
> > > 
> > > -	md5=`hexdump -e '16/1 "%_p" "\n"' $testfile | md5sum`
> > > -	printf "#%d %s\n" "$block" "$md5"
> > > +	echo -n "#$block "
> > > +	md5sum $testfile | _filter_test_dir
> > 
> > Yes, I thought about that too, but I can't be sure about it, so tried to keep
> > the logic of original code. If the original author doesn't have some special
> > reason, I'd like to change it as this way (cc roman.penyaev@profitbricks.com).
> > 
> > And for the generic/042:
> > -       if hexdump -v -e '/1 "%02X "' $file | grep -q "CD"; then
> > +       if od -An -tx1 -v $file | grep -q "CD"; then
> > 
> > I don't know what that "-v" option is needed (cc Darrick). I thought it might just
> > waste the time of grep running ?
> 
> Probably.  I probably put that in there for debugging purposes and then
> frogot to take it out.

Thanks Darrick, I'll remove it in V2 patch.

I tried to ask roman.penyaev@profitbricks.com, but that email can't be reached. So
I'll change g/404 as we talked.

> 
> --D
> 
> > > 
> > > Seperate patch, perhaps?
> > 
> > Sure, if no one object that, I'll change them in separated patch.
> > 
> > Thanks,
> > Zorro
> > 
> > > 
> > > Cheers,
> > > 
> > > Dave.
> > > -- 
> > > Dave Chinner
> > > david@fromorbit.com
> > > 
> > 
>