| Message ID | 1466678252-4058-1-git-send-email-anand.jain@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jun 23, 2016 at 06:37:32PM +0800, Anand Jain wrote: > btrfs fi sync /mnt, now does not output anything for success, > so the 006.out should be updated. > > This change in btrfs-progs was introduced in the commit > b005ca024990569d2de459485682158633937928 > btrfs-progs: fi sync: make it silent by default > which was integrated at btrfs-progs version v4.5.2 Thanks! > > Further created helper function _runnt_btrfs_utils_progs() > which won't call _fail upon command failure, instead it just > echo to stdout. This was required so to continue with the > test script and do the cleanups at the end. > > Signed-off-by: Anand Jain <anand.jain@oracle.com> > --- > v2: Update commit log to indicate the btrfs-progs version at > which sync UI is changed. > > common/rc | 11 +++++++++++ > tests/btrfs/006 | 2 +- > tests/btrfs/006.out | 1 - > 3 files changed, 12 insertions(+), 2 deletions(-) > > 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? Thanks, Eryu > > echo "== Show device stats by mountpoint" > $BTRFS_UTIL_PROG device stats $SCRATCH_MNT | _filter_btrfs_device_stats $TOTAL_DEVS > diff --git a/tests/btrfs/006.out b/tests/btrfs/006.out > index 22bcb777076a..05b9ac020737 100644 > --- a/tests/btrfs/006.out > +++ b/tests/btrfs/006.out > @@ -14,7 +14,6 @@ Label: 'TestLabel.006' uuid: <EXACTUUID> > devid <DEVID> size <SIZE> used <SIZE> path SCRATCH_DEV > > == Sync filesystem > -FSSync 'SCRATCH_MNT' > == Show device stats by mountpoint > <NUMDEVS> [SCRATCH_DEV].corruption_errs <NUM> > <NUMDEVS> [SCRATCH_DEV].flush_io_errs <NUM> > -- > 2.7.0 > > -- > To unsubscribe from this list: send the line "unsubscribe fstests" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe fstests" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/23/2016 06:53 PM, Eryu Guan wrote: > On Thu, Jun 23, 2016 at 06:37:32PM +0800, Anand Jain wrote: >> btrfs fi sync /mnt, now does not output anything for success, >> so the 006.out should be updated. >> >> This change in btrfs-progs was introduced in the commit >> b005ca024990569d2de459485682158633937928 >> btrfs-progs: fi sync: make it silent by default >> which was integrated at btrfs-progs version v4.5.2 > > Thanks! > >> >> Further created helper function _runnt_btrfs_utils_progs() >> which won't call _fail upon command failure, instead it just >> echo to stdout. This was required so to continue with the >> test script and do the cleanups at the end. >> >> Signed-off-by: Anand Jain <anand.jain@oracle.com> >> --- >> v2: Update commit log to indicate the btrfs-progs version at >> which sync UI is changed. >> >> common/rc | 11 +++++++++++ >> tests/btrfs/006 | 2 +- >> tests/btrfs/006.out | 1 - >> 3 files changed, 12 insertions(+), 2 deletions(-) >> >> 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. Thanks, Anand > Thanks, > Eryu >> >> echo "== Show device stats by mountpoint" >> $BTRFS_UTIL_PROG device stats $SCRATCH_MNT | _filter_btrfs_device_stats $TOTAL_DEVS >> diff --git a/tests/btrfs/006.out b/tests/btrfs/006.out >> index 22bcb777076a..05b9ac020737 100644 >> --- a/tests/btrfs/006.out >> +++ b/tests/btrfs/006.out >> @@ -14,7 +14,6 @@ Label: 'TestLabel.006' uuid: <EXACTUUID> >> devid <DEVID> size <SIZE> used <SIZE> path SCRATCH_DEV >> >> == Sync filesystem >> -FSSync 'SCRATCH_MNT' >> == Show device stats by mountpoint >> <NUMDEVS> [SCRATCH_DEV].corruption_errs <NUM> >> <NUMDEVS> [SCRATCH_DEV].flush_io_errs <NUM> >> -- >> 2.7.0 >> >> -- >> To unsubscribe from this list: send the line "unsubscribe fstests" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.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 > -- To unsubscribe from this list: send the line "unsubscribe fstests" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jun 23, 2016 at 07:03:59PM +0800, Anand Jain wrote: > > > On 06/23/2016 06:53 PM, Eryu Guan wrote: [snip] > > > 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. Thanks, Eryu -- To unsubscribe from this list: send the line "unsubscribe fstests" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/23/2016 07:18 PM, Eryu Guan wrote: > On Thu, Jun 23, 2016 at 07:03:59PM +0800, Anand Jain wrote: >> >> >> On 06/23/2016 06:53 PM, Eryu Guan wrote: > [snip] >>>> 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. Thanks, Anand > Thanks, > Eryu > -- > 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 > -- To unsubscribe from this list: send the line "unsubscribe fstests" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jun 23, 2016 at 12:34 PM, Anand Jain <anand.jain@oracle.com> wrote: > > > On 06/23/2016 07:18 PM, Eryu Guan wrote: >> >> On Thu, Jun 23, 2016 at 07:03:59PM +0800, Anand Jain wrote: >>> >>> >>> >>> On 06/23/2016 06:53 PM, Eryu Guan wrote: >> >> [snip] >>>>> >>>>> 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. That's precisely why both error messages to stderr *and* return values are used. For scripts you always have the option to ignore stderr through redirection to /dev/null and check only the return value. Also _runnt_btrfs_util_prog filesystem is, in my opinion, a terrible name. What does "runnt" means? > > Thanks, Anand > > >> Thanks, >> Eryu >> -- >> 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 >> > -- > To unsubscribe from this list: send the line "unsubscribe fstests" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/23/2016 07:54 PM, Filipe Manana wrote: > On Thu, Jun 23, 2016 at 12:34 PM, Anand Jain <anand.jain@oracle.com> wrote: >> >> >> On 06/23/2016 07:18 PM, Eryu Guan wrote: >>> >>> On Thu, Jun 23, 2016 at 07:03:59PM +0800, Anand Jain wrote: >>>> >>>> >>>> >>>> On 06/23/2016 06:53 PM, Eryu Guan wrote: >>> >>> [snip] >>>>>> >>>>>> 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. > > That's precisely why both error messages to stderr *and* return values are used. > For scripts you always have the option to ignore stderr through > redirection to /dev/null and check only the return value. > > Also _runnt_btrfs_util_prog filesystem is, in my opinion, a terrible > name. What does "runnt" means? run (but) not terminate. Thanks, Anand >> >> Thanks, Anand >> >> >>> Thanks, >>> Eryu >>> -- >>> 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 >>> >> -- >> To unsubscribe from this list: send the line "unsubscribe fstests" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html > > > -- To unsubscribe from this list: send the line "unsubscribe fstests" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jun 23, 2016 at 07:34:49PM +0800, Anand Jain wrote: > > > On 06/23/2016 07:18 PM, Eryu Guan wrote: > > On Thu, Jun 23, 2016 at 07:03:59PM +0800, Anand Jain wrote: > > > > > > > > > On 06/23/2016 06:53 PM, Eryu Guan wrote: > > [snip] > > > > > 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. 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 fstests" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/23/2016 08:11 PM, Eryu Guan wrote: > On Thu, Jun 23, 2016 at 07:34:49PM +0800, Anand Jain wrote: >> >> >> On 06/23/2016 07:18 PM, Eryu Guan wrote: >>> On Thu, Jun 23, 2016 at 07:03:59PM +0800, Anand Jain wrote: >>>> >>>> >>>> On 06/23/2016 06:53 PM, Eryu Guan wrote: >>> [snip] >>>>>> 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 > -- To unsubscribe from this list: send the line "unsubscribe fstests" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 echo "== Show device stats by mountpoint" $BTRFS_UTIL_PROG device stats $SCRATCH_MNT | _filter_btrfs_device_stats $TOTAL_DEVS diff --git a/tests/btrfs/006.out b/tests/btrfs/006.out index 22bcb777076a..05b9ac020737 100644 --- a/tests/btrfs/006.out +++ b/tests/btrfs/006.out @@ -14,7 +14,6 @@ Label: 'TestLabel.006' uuid: <EXACTUUID> devid <DEVID> size <SIZE> used <SIZE> path SCRATCH_DEV == Sync filesystem -FSSync 'SCRATCH_MNT' == Show device stats by mountpoint <NUMDEVS> [SCRATCH_DEV].corruption_errs <NUM> <NUMDEVS> [SCRATCH_DEV].flush_io_errs <NUM>
btrfs fi sync /mnt, now does not output anything for success, so the 006.out should be updated. This change in btrfs-progs was introduced in the commit b005ca024990569d2de459485682158633937928 btrfs-progs: fi sync: make it silent by default which was integrated at btrfs-progs version v4.5.2 Further created helper function _runnt_btrfs_utils_progs() which won't call _fail upon command failure, instead it just echo to stdout. This was required so to continue with the test script and do the cleanups at the end. Signed-off-by: Anand Jain <anand.jain@oracle.com> --- v2: Update commit log to indicate the btrfs-progs version at which sync UI is changed. common/rc | 11 +++++++++++ tests/btrfs/006 | 2 +- tests/btrfs/006.out | 1 - 3 files changed, 12 insertions(+), 2 deletions(-)