[PATCH v2] fstests: btrfs: fix 006 adds _runnt_btrfs_util_prog()

[PATCH v2] fstests: btrfs: fix 006 adds _runnt_btrfs_util_prog()

[Anand Jain]

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(-)

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>
-- 
2.7.0

Re: [PATCH v2] fstests: btrfs: fix 006 adds _runnt_btrfs_util_prog()

[Eryu Guan]

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

Re: [PATCH v2] fstests: btrfs: fix 006 adds _runnt_btrfs_util_prog()

[Anand Jain]

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
>

Re: [PATCH v2] fstests: btrfs: fix 006 adds _runnt_btrfs_util_prog()

[Eryu Guan]

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

Re: [PATCH v2] fstests: btrfs: fix 006 adds _runnt_btrfs_util_prog()

[Anand Jain]

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
>

Re: [PATCH v2] fstests: btrfs: fix 006 adds _runnt_btrfs_util_prog()

[Filipe Manana]

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



-- 
Filipe David Manana,

"Reasonable men adapt themselves to the world.
 Unreasonable men adapt the world to themselves.
 That's why all progress depends on unreasonable men."

Re: [PATCH v2] fstests: btrfs: fix 006 adds _runnt_btrfs_util_prog()

[Anand Jain]

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
>
>
>

Re: [PATCH v2] fstests: btrfs: fix 006 adds _runnt_btrfs_util_prog()

[Eryu Guan]

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

Re: [PATCH v2] fstests: btrfs: fix 006 adds _runnt_btrfs_util_prog()

[Anand Jain]

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
>

[PATCH v3] fstests: btrfs: fix 006

[Anand Jain]

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

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.
 v3: Drop _runnt_btrfs_utils_progs(). fi sync will error stdout
     on error, so don't need. Thanks Eryu.

 tests/btrfs/006     | 2 +-
 tests/btrfs/006.out | 1 -
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/tests/btrfs/006 b/tests/btrfs/006
index 715fd80fb6fc..086339401153 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
+$BTRFS_UTIL_PROG filesystem sync $SCRATCH_MNT > /dev/null
 
 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