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@xxxxxxxxxx> > Signed-off-by: Qu Wenruo <quwenruo@xxxxxxxxxxxxxx> > --- > 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@xxxxxxxxxxxxx -- To unsubscribe from this list: send the line "unsubscribe fstests" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
