On 06/23/2016 08:11 PM, Eryu Guan wrote:

On Thu, Jun 23, 2016 at 07:34:49PM +0800, Anand Jain wrote:

quoted

On 06/23/2016 07:18 PM, Eryu Guan wrote:

quoted

On Thu, Jun 23, 2016 at 07:03:59PM +0800, Anand Jain wrote:

quoted

On 06/23/2016 06:53 PM, Eryu Guan wrote:

[snip]

quoted

quoted

quoted

diff --git a/common/rc b/common/rc
index a44fb8750220..2a10fbb2d341 100644
--- a/common/rc
+++ b/common/rc

@@ -3114,6 +3114,17 @@ _min_dio_alignment()
     fi
 }

+run_check_dontfail()
+{
+	echo "# $@" >> $seqres.full 2>&1
+	"$@" >> $seqres.full 2>&1 || echo "failed: '$@'"
+}
+
+_runnt_btrfs_util_prog()
+{
+	run_check_dontfail $BTRFS_UTIL_PROG $*
+}
+
 run_check()
 {
 	echo "# $@" >> $seqres.full 2>&1

diff --git a/tests/btrfs/006 b/tests/btrfs/006
index 715fd80fb6fc..9d1fe09e07de 100755
--- a/tests/btrfs/006
+++ b/tests/btrfs/006

@@ -79,7 +79,7 @@ echo "== Show filesystem by UUID"
 $BTRFS_UTIL_PROG filesystem show $UUID | _filter_btrfs_filesystem_show $TOTAL_DEVS $UUID

 echo "== Sync filesystem"
-$BTRFS_UTIL_PROG filesystem sync $SCRATCH_MNT | _filter_scratch
+_runnt_btrfs_util_prog filesystem sync $SCRATCH_MNT

Still, I don't think this helper is necessary.

$BTRFS_UTIL_PROG filesystem sync $SCRATCH_MNT >/dev/null

doesn't _fail on failure, output error messages breaks golden image, and
is much simpler. Do I miss anything?

 runnt_btrfs_util_prog() checks the return status of the command,
 if failed (!0) it will echo so to break the golden image.

So it fails silently now? (return non-zero value and print no error
message) That seems a btrfs-progs bug to me.. It should print error
messages to stderr on failure, so we don't have to check the return
value explicitly.

 For programming interfaces I would rather depend more on the return
 value, than the UI/error strings.

I checked btrfs-progs code, it does print error message on failure. So
the question is whether should we depend on return value of commands.

fstests is designed to let users to ignore return value and depend on
error messages to break golden image, so we don't have to check every
command's return value. Quating Dave's previous review [1]:

"
the test harness infrastructure is designed specifcally so
that we don't need to check the error status of every program we
run.  Programs need to give users obvious feedback of failure (i.e.
stdout/stderr) because users *do not check return codes*, and the
test harness is designed around ensuring programs generate useful
error messages.
"

And the usage of run_check is not encouraged[2], and even is considered
harmful [3].

So please depend on error messages in fstests and avoid using helpers
like run_check.

  Ah. I didn't know this thanks, v3 is on its way.

Thanks, Anand

Thanks,
Eryu

[1] http://www.spinics.net/lists/fstests/msg01333.html
[2] http://www.spinics.net/lists/fstests/msg01299.html
[3] http://www.spinics.net/lists/fstests/msg01489.html
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html