[PATCH] fstest: btrfs/006: Add extra check on return value and 'fi show' by device

[PATCH] fstest: btrfs/006: Add extra check on return value and 'fi show' by device

[Qu Wenruo]

Reported in Red Hat BZ#1181627, 'btrfs fi show' on unmounted device will
return 1 even no error happens.

Introduced by: commit 2513077f
btrfs-progs: fix device missing of btrfs fi show with seed devices

Patch fixing it:
https://patchwork.kernel.org/patch/5626001/
btrfs-progs: Fix wrong return value when executing 'fi show' on
umounted device.

Reported-by: Vratislav Podzimek <vpodzime@redhat.com>
Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
---
 tests/btrfs/006     | 51 ++++++++++++++++++++++++++++++++++++++++++++-------
 tests/btrfs/006.out | 10 ++++++++++
 2 files changed, 54 insertions(+), 7 deletions(-)

diff --git a/tests/btrfs/006 b/tests/btrfs/006
index 715fd80..2d8c1c0 100755
--- a/tests/btrfs/006
+++ b/tests/btrfs/006
@@ -62,33 +62,70 @@ _scratch_pool_mkfs >> $seqres.full 2>&1 || _fail "mkfs failed"
 
 # These have to be done unmounted...?
 echo "== Set filesystem label to $LABEL"
-$BTRFS_UTIL_PROG filesystem label $SCRATCH_DEV $LABEL
+$BTRFS_UTIL_PROG filesystem label $SCRATCH_DEV $LABEL || \
+	_fail "set lable failed"
 echo "== Get filesystem label"
-$BTRFS_UTIL_PROG filesystem label $SCRATCH_DEV
+$BTRFS_UTIL_PROG filesystem label $SCRATCH_DEV || \
+	_fail "get lable failed"
+
+echo "== Show filesystem by device(offline)"
+$BTRFS_UTIL_PROG filesystem show $FIRST_POOL_DEV | \
+	_filter_btrfs_filesystem_show $TOTAL_DEVS $UUID
+[ ${PIPESTATUS[0]} -ne 0 ] && \
+	_fail "show filesystem by device(offline) return value wrong"
 
 echo "== Mount."
 _scratch_mount
 
 echo "== Show filesystem by label"
-$BTRFS_UTIL_PROG filesystem show $LABEL | _filter_btrfs_filesystem_show $TOTAL_DEVS
+$BTRFS_UTIL_PROG filesystem show $LABEL | \
+	_filter_btrfs_filesystem_show $TOTAL_DEVS
+[ ${PIPESTATUS[0]} -ne 0 ] && \
+	_fail "show filesystem by lable return value wrong"
 UUID=`$BTRFS_UTIL_PROG filesystem show $LABEL | grep uuid: | awk '{print $NF}'`
 
 echo "UUID $UUID" >> $seqres.full
 
 echo "== Show filesystem by UUID"
-$BTRFS_UTIL_PROG filesystem show $UUID | _filter_btrfs_filesystem_show $TOTAL_DEVS $UUID
+$BTRFS_UTIL_PROG filesystem show $UUID | \
+	_filter_btrfs_filesystem_show $TOTAL_DEVS $UUID
+[ ${PIPESTATUS[0]} -ne 0 ] && \
+	_fail "show filesystem by UUID return value wrong"
+
+echo "== Show filesystem by device(online)"
+$BTRFS_UTIL_PROG filesystem show $FIRST_POOL_DEV | \
+	_filter_btrfs_filesystem_show $TOTAL_DEVS $UUID
+[ ${PIPESTATUS[0]} -ne 0 ] && \
+	_fail "show filesystem by UUID return value wrong"
 
 echo "== Sync filesystem"
 $BTRFS_UTIL_PROG filesystem sync $SCRATCH_MNT | _filter_scratch
+[ ${PIPESTATUS[0]} -ne 0 ] && \
+	_fail "sync filesystem failed"
+
 
 echo "== Show device stats by mountpoint"
-$BTRFS_UTIL_PROG device stats $SCRATCH_MNT | _filter_btrfs_device_stats $TOTAL_DEVS
+$BTRFS_UTIL_PROG device stats $SCRATCH_MNT | \
+	_filter_btrfs_device_stats $TOTAL_DEVS
+[ ${PIPESTATUS[0]} -ne 0 ] && \
+	_fail "show device status return value wrong"
+
 echo "== Show device stats by first/scratch dev"
 $BTRFS_UTIL_PROG device stats $SCRATCH_DEV | _filter_btrfs_device_stats
+[ ${PIPESTATUS[0]} -ne 0 ] && \
+	_fail "show device status return value wrong"
+
 echo "== Show device stats by second dev"
-$BTRFS_UTIL_PROG device stats $FIRST_POOL_DEV | sed -e "s,$FIRST_POOL_DEV,FIRST_POOL_DEV,g"
+$BTRFS_UTIL_PROG device stats $FIRST_POOL_DEV | \
+	sed -e "s,$FIRST_POOL_DEV,FIRST_POOL_DEV,g"
+[ ${PIPESTATUS[0]} -ne 0 ] && \
+	_fail "show device status return value wrong"
+
 echo "== Show device stats by last dev"
-$BTRFS_UTIL_PROG device stats $LAST_POOL_DEV | sed -e "s,$LAST_POOL_DEV,LAST_POOL_DEV,g"
+$BTRFS_UTIL_PROG device stats $LAST_POOL_DEV | \
+	sed -e "s,$LAST_POOL_DEV,LAST_POOL_DEV,g"
+[ ${PIPESTATUS[0]} -ne 0 ] && \
+	_fail "show device status return value wrong"
 
 # success, all done
 status=0
diff --git a/tests/btrfs/006.out b/tests/btrfs/006.out
index 22bcb77..497de67 100644
--- a/tests/btrfs/006.out
+++ b/tests/btrfs/006.out
@@ -2,6 +2,11 @@
 == Set filesystem label to TestLabel.006
 == Get filesystem label
 TestLabel.006
+== Show filesystem by device(offline)
+Label: 'TestLabel.006'  uuid: <UUID>
+	Total devices <EXACTNUM> FS bytes used <SIZE>
+	devid <DEVID> size <SIZE> used <SIZE> path SCRATCH_DEV
+
 == Mount.
 == Show filesystem by label
 Label: 'TestLabel.006'  uuid: <UUID>
@@ -13,6 +18,11 @@ Label: 'TestLabel.006'  uuid: <EXACTUUID>
 	Total devices <EXACTNUM> FS bytes used <SIZE>
 	devid <DEVID> size <SIZE> used <SIZE> path SCRATCH_DEV
 
+== Show filesystem by device(online)
+Label: 'TestLabel.006'  uuid: <EXACTUUID>
+	Total devices <EXACTNUM> FS bytes used <SIZE>
+	devid <DEVID> size <SIZE> used <SIZE> path SCRATCH_DEV
+
 == Sync filesystem
 FSSync 'SCRATCH_MNT'
 == Show device stats by mountpoint
-- 
2.2.2

Re: [PATCH] fstest: btrfs/006: Add extra check on return value and 'fi show' by device

[Dave Chinner]

On Fri, Jan 16, 2015 at 09:57:10AM +0800, Qu Wenruo wrote:
> Reported in Red Hat BZ#1181627, 'btrfs fi show' on unmounted device will
> return 1 even no error happens.

Please describe the bug in the commit message, don't make me have to
go look at some web page to work out what you are trying to fix.

> Introduced by: commit 2513077f
> btrfs-progs: fix device missing of btrfs fi show with seed devices
> 
> Patch fixing it:
> https://patchwork.kernel.org/patch/5626001/
> btrfs-progs: Fix wrong return value when executing 'fi show' on
> umounted device.

What btrfs progs commit introduced and fixed is not actually
relevant to fstests. If I have to go read other stuff to understand
the change you are making, then the description needs more detail
and less external references....


> Reported-by: Vratislav Podzimek <vpodzime@redhat.com>
> Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
> ---
>  tests/btrfs/006     | 51 ++++++++++++++++++++++++++++++++++++++++++++-------
>  tests/btrfs/006.out | 10 ++++++++++
>  2 files changed, 54 insertions(+), 7 deletions(-)
> 
> diff --git a/tests/btrfs/006 b/tests/btrfs/006
> index 715fd80..2d8c1c0 100755
> --- a/tests/btrfs/006
> +++ b/tests/btrfs/006
> @@ -62,33 +62,70 @@ _scratch_pool_mkfs >> $seqres.full 2>&1 || _fail "mkfs failed"
>  
>  # These have to be done unmounted...?
>  echo "== Set filesystem label to $LABEL"
> -$BTRFS_UTIL_PROG filesystem label $SCRATCH_DEV $LABEL
> +$BTRFS_UTIL_PROG filesystem label $SCRATCH_DEV $LABEL || \
> +	_fail "set lable failed"

Apart from the typo, why not _run_btrfs_util_prog?

However, this will make the test fail on older btrfs progs
installations that we previously passing the test just fine, right?

IOWs, if you want to test that `btrfs fi show` always returns the
correct value, don't add that checking to an existing test: write a
new test that tests the validity of the return value....


>  echo "== Get filesystem label"
> -$BTRFS_UTIL_PROG filesystem label $SCRATCH_DEV
> +$BTRFS_UTIL_PROG filesystem label $SCRATCH_DEV || \
> +	_fail "get lable failed"
> +
> +echo "== Show filesystem by device(offline)"
> +$BTRFS_UTIL_PROG filesystem show $FIRST_POOL_DEV | \
> +	_filter_btrfs_filesystem_show $TOTAL_DEVS $UUID
> +[ ${PIPESTATUS[0]} -ne 0 ] && \
> +	_fail "show filesystem by device(offline) return value wrong"

That's a new function being tested. Again, this belongs in a new
test, not an existing regression test....

FWIW, Now that I've seen it used a couple of times, I don't really
like the mess that checking errors via PIPESTATUS results in,
especially as

_run_btrfs_util_prog filesystem show $FIRST_POOL_DEV | \
	_filter_btrfs_filesystem_show $TOTAL_DEVS $UUID

Gives *exactly* the same failure detection....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] fstest: btrfs/006: Add extra check on return value and 'fi show' by device

[Qu Wenruo]

-------- Original Message --------
Subject: Re: [PATCH] fstest: btrfs/006: Add extra check on return value 
and 'fi show' by device
From: Dave Chinner <david@fromorbit.com>
To: Qu Wenruo <quwenruo@cn.fujitsu.com>
Date: 2015年01月21日 12:03
> On Fri, Jan 16, 2015 at 09:57:10AM +0800, Qu Wenruo wrote:
>> Reported in Red Hat BZ#1181627, 'btrfs fi show' on unmounted device will
>> return 1 even no error happens.
> Please describe the bug in the commit message, don't make me have to
> go look at some web page to work out what you are trying to fix.
OK, I'll make the description more detailed.
>
>> Introduced by: commit 2513077f
>> btrfs-progs: fix device missing of btrfs fi show with seed devices
>>
>> Patch fixing it:
>> https://patchwork.kernel.org/patch/5626001/
>> btrfs-progs: Fix wrong return value when executing 'fi show' on
>> umounted device.
> What btrfs progs commit introduced and fixed is not actually
> relevant to fstests.
This just saves some time for some users who failed the test and what to 
check if the bug is fixed or not.
> If I have to go read other stuff to understand
> the change you are making, then the description needs more detail
> and less external references....
I understand that I need to make the description more detailed.
Although external reference does not help much for fstests, but it will 
really saves time
for some tests.
Like Fillipe Manana's test, which embedded such info into the test 
script, saved me a lot of
time to find the regression commit.
>
>
>> Reported-by: Vratislav Podzimek <vpodzime@redhat.com>
>> Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
>> ---
>>   tests/btrfs/006     | 51 ++++++++++++++++++++++++++++++++++++++++++++-------
>>   tests/btrfs/006.out | 10 ++++++++++
>>   2 files changed, 54 insertions(+), 7 deletions(-)
>>
>> diff --git a/tests/btrfs/006 b/tests/btrfs/006
>> index 715fd80..2d8c1c0 100755
>> --- a/tests/btrfs/006
>> +++ b/tests/btrfs/006
>> @@ -62,33 +62,70 @@ _scratch_pool_mkfs >> $seqres.full 2>&1 || _fail "mkfs failed"
>>   
>>   # These have to be done unmounted...?
>>   echo "== Set filesystem label to $LABEL"
>> -$BTRFS_UTIL_PROG filesystem label $SCRATCH_DEV $LABEL
>> +$BTRFS_UTIL_PROG filesystem label $SCRATCH_DEV $LABEL || \
>> +	_fail "set lable failed"
> Apart from the typo, why not _run_btrfs_util_prog?
Oh thanks a lot for the existing infrastructure.

With this one, the PIPESTATUS[] is no longer needed.
>
> However, this will make the test fail on older btrfs progs
> installations that we previously passing the test just fine, right?
No, old tests will still pass, since it returns 0 as expected.
Add the return values test just in case.
>
> IOWs, if you want to test that `btrfs fi show` always returns the
> correct value, don't add that checking to an existing test: write a
> new test that tests the validity of the return value....
OK, creating a new one seems reasonable enough for me.
>
>
>>   echo "== Get filesystem label"
>> -$BTRFS_UTIL_PROG filesystem label $SCRATCH_DEV
>> +$BTRFS_UTIL_PROG filesystem label $SCRATCH_DEV || \
>> +	_fail "get lable failed"
>> +
>> +echo "== Show filesystem by device(offline)"
>> +$BTRFS_UTIL_PROG filesystem show $FIRST_POOL_DEV | \
>> +	_filter_btrfs_filesystem_show $TOTAL_DEVS $UUID
>> +[ ${PIPESTATUS[0]} -ne 0 ] && \
>> +	_fail "show filesystem by device(offline) return value wrong"
> That's a new function being tested. Again, this belongs in a new
> test, not an existing regression test....
>
> FWIW, Now that I've seen it used a couple of times, I don't really
> like the mess that checking errors via PIPESTATUS results in,
> especially as
>
> _run_btrfs_util_prog filesystem show $FIRST_POOL_DEV | \
> 	_filter_btrfs_filesystem_show $TOTAL_DEVS $UUID
>
> Gives *exactly* the same failure detection....
Thanks for the _run_btrfs_util_prog macro hint. That's great and makes 
codes look much nicer.

BTW, do I need to cleanup all the $BTRFS_UTIL_PROG in the original test?

Thanks,
Qu
>
> Cheers,
>
> Dave.

Re: [PATCH] fstest: btrfs/006: Add extra check on return value and 'fi show' by device

[Dave Chinner]

On Wed, Jan 21, 2015 at 12:19:32PM +0800, Qu Wenruo wrote:
> From: Dave Chinner <david@fromorbit.com>
> >On Fri, Jan 16, 2015 at 09:57:10AM +0800, Qu Wenruo wrote:
> >>Reported in Red Hat BZ#1181627, 'btrfs fi show' on unmounted device will
> >>return 1 even no error happens.
> >Please describe the bug in the commit message, don't make me have to
> >go look at some web page to work out what you are trying to fix.
> OK, I'll make the description more detailed.
> >
> >>Introduced by: commit 2513077f
> >>btrfs-progs: fix device missing of btrfs fi show with seed devices
> >>
> >>Patch fixing it:
> >>https://patchwork.kernel.org/patch/5626001/
> >>btrfs-progs: Fix wrong return value when executing 'fi show' on
> >>umounted device.
> >What btrfs progs commit introduced and fixed is not actually
> >relevant to fstests.
> This just saves some time for some users who failed the test and
> what to check if the bug is fixed or not.

Doesn't save me time - it takes me much longer as I have to use 3
different tools to follow those three references to try to
understand why you are making the change.

> >If I have to go read other stuff to understand
> >the change you are making, then the description needs more detail
> >and less external references....

> I understand that I need to make the description more detailed.
> Although external reference does not help much for fstests, but it
> will really saves time for some tests.  Like Fillipe Manana's
> test, which embedded such info into the test script, saved me a
> lot of time to find the regression commit.

What it helps is people running a new fstests tree on an older
distro kernels. It doesn't save time for reviewers, yet reviewer
resources are in much scarcer supply than distro QA resources.
If you want your changes reviewed faster, then commit messages need
to be reviewer friendly. Further, if you review other people's
changes, then they are much more likely to review your changes
quickly.

If the review burden is left to me all the time then, like Andrew
Morton, I get grumpy when people say "but it helps me".....

> BTW, do I need to cleanup all the $BTRFS_UTIL_PROG in the original test?

Not if it causes older btrfs progs installations to fail.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com