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

["Darrick J. Wong" <djwong@kernel.org>]

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.

--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
> > 
>