Re: [PATCH] ext4/004: add dump/restore test

[Dave Chinner <david@fromorbit.com>]

On Tue, Nov 25, 2014 at 06:21:09PM +0800, Xiaoguang Wang wrote:
> This test case will first use fsstress to fill a file system, then
> dump it to standard output and restore it from standard input, finally
> check that the original contents and the new contents generated by
> restore tool will be same.
> 
> Signed-off-by: Xiaoguang Wang <wangxg.fnst@cn.fujitsu.com>
> ---
>  common/config      |   2 ++
>  common/rc          |   7 ++++
>  tests/ext4/004     | 104 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  tests/ext4/004.out |   7 ++++
>  tests/ext4/group   |   3 +-
>  5 files changed, 122 insertions(+), 1 deletion(-)
>  create mode 100755 tests/ext4/004
>  create mode 100644 tests/ext4/004.out
> 
> diff --git a/common/config b/common/config
> index 1cb08c0..8ea70ec 100644
> --- a/common/config
> +++ b/common/config
> @@ -188,6 +188,8 @@ export LOGGER_PROG="`set_prog_path logger`"
>  export DBENCH_PROG="`set_prog_path dbench`"
>  export DMSETUP_PROG="`set_prog_path dmsetup`"
>  export WIPEFS_PROG="`set_prog_path wipefs`"
> +export E2FS_DUMP_PROG="`set_prog_path dump`"
> +export E2FS_RESTORE_PROG="`set_prog_path restore`"

So now we have DUMPE2FS_PROG and E2FS_DUMP_PROG, which are different
programs altogether but are going to be very easy to confuse.

I would suggest that follow the convention of all the other *_PROG
variables dump -> $DUMP_PROG is probably a good idea to avoid
confusion.

>  # Generate a comparable xfsprogs version number in the form of
>  # major * 10000 + minor * 100 + release
> diff --git a/common/rc b/common/rc
> index d5e3aff..ef7939d 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -2258,6 +2258,13 @@ _require_fio()
>  	[ $? -eq 0 ] || _notrun "$FIO_PROG too old, see $seqres.full"
>  }
>  
> +# Check that dump/restore is present
> +_require_dump_restore()
> +{
> +	_require_command $E2FS_DUMP_PROG "dump"
> +	_require_command $E2FS_RESTORE_PROG "restore"
> +}

No need for the wrapper, just call _require_command directly.

> +
>  # Does freeze work on this fs?
>  _require_freeze()
>  {
> diff --git a/tests/ext4/004 b/tests/ext4/004
> new file mode 100755
> index 0000000..cbf35e5
> --- /dev/null
> +++ b/tests/ext4/004
> @@ -0,0 +1,104 @@
> +#! /bin/bash
> +# FSQA Test No. 004
> +#
> +# Test "dump | restore"(as opposed to a tape)
> +#
> +#-----------------------------------------------------------------------
> +# Copyright (c) 2014 Fujitsu.  All Rights Reserved.
> +#
> +# This program is free software; you can redistribute it and/or
> +# modify it under the terms of the GNU General Public License as
> +# published by the Free Software Foundation.
> +#
> +# This program is distributed in the hope that it would be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program; if not, write the Free Software Foundation,
> +# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> +#
> +#-----------------------------------------------------------------------
> +#
> +
> +seq=`basename $0`
> +seqres=$RESULT_DIR/$seq
> +echo "QA output created by $seq"
> +
> +here=`pwd`
> +tmp=/tmp/$$
> +status=1	# failure is the default!
> +trap "_cleanup; exit \$status" 0 1 2 3 15
> +
> +_cleanup()
> +{
> +	cd /
> +	rm -f $tmp.*
> +	rm -rf $restore_dir
> +	rm -rf $dump_dir
> +	_scratch_unmount

scratch does not need to be unmounted here, nor does it need to be
cleaned up. Also, you have to assume that the previous run of the
test did not clean up (i.e. system crashed before cleanup ran),
so you are going to want to do the rm on $restore_dir as part of the
test setup, not cleanup....

> +}
> +
> +# get standard environment, filters and checks
> +. ./common/rc
> +. ./common/filter
> +
> +dump_dir=$SCRATCH_MNT/dump_restore_dir
> +restore_dir=$TEST_DIR/dump_restore_dir
> +
> +_diff_compare()

"_" prefix is for library functions (i.e. from common/...), not
local functions.

> +{
> +	echo "Comparing dump directory with restore directory"
> +	diff -rs $dump_dir $restore_dir >> $seqres.full 2>&1
> +	if [ $? -ne 0 ];then
> +		echo "Dump/Restore compare failed"
> +		exit
> +	fi
> +	echo "Dump/Restore compare success"
> +}

There is no need for this entire function. just running:

	diff -r $dump_dir $restore_dir

will tell us everything. i.e. it is silent if both directories are
the same, but will throw output if there are differences. That will
output will cause the golden image match (and hence the test) to fail....

> +
> +_workout()
> +{
> +	echo ""
> +	echo "Run fsstress"
> +	args=`_scale_fsstress_args -z -f creat=5 -f write=20 -f mkdir=5 -n 1000 -p 15 -d $dump_dir`
> +	echo "fsstress $args" >> $seqres.full
> +
> +	$FSSTRESS_PROG $args >> $seqres.full 2>&1
> +
> +	echo "start Dump/Restore"
> +	cd $TEST_DIR
> +
> +	$E2FS_DUMP_PROG	-0 -f - $dump_dir 2>/dev/null | $E2FS_RESTORE_PROG -urvf - >> $seqres.full 2>&1
> +	if [ $? -ne 0 ];then
> +		echo "Dump/Restore failed"
> +		exit

_fail?

> +	else
> +		echo "Dump/Restore success"
> +	fi

And there's no need to echo anything on success.

> +	rm -rf restoresymtable
> +	_diff_compare
> +}

No need to put all of this in a function.

> +# real QA test starts here
> +_supported_fs ext4
> +_supported_os Linux
> +
> +_require_scratch
> +_require_dump_restore

_require_test

> +
> +rm -f $seqres.full
> +
> +_scratch_mkfs_sized $((512 * 1024 * 1024)) >> $seqres.full 2>&1
> +_scratch_mount || _fail "couldn't mount fs"

No need to check for mount failure.

> +
> +umount $TEST_DEV
> +_test_mkfs >> $seqres.full 2>&1
> +_test_mount

No. That is just not allowed. The test device should *never* be
mkfs'd by a test. It shouldn't even be unmounted. Prep should be 
just an rm...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com