diff mbox

btrfs: add regression test for setxattr on subvolume directory

Message ID 28ac6254ad926413836489a46cbf14497f4802ed.1485327013.git.osandov@fb.com (mailing list archive)
State New, archived
Headers show

Commit Message

Omar Sandoval Jan. 25, 2017, 6:50 a.m. UTC
From: Omar Sandoval <osandov@fb.com>

This is a regression test for "Btrfs: disable xattr operations on
subvolume directories". On v4.9, it will result in an aborted
transaction.

Signed-off-by: Omar Sandoval <osandov@fb.com>
---
 tests/btrfs/047     | 69 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 tests/btrfs/047.out |  2 ++
 tests/btrfs/group   |  1 +
 3 files changed, 72 insertions(+)
 create mode 100644 tests/btrfs/047
 create mode 100644 tests/btrfs/047.out

Comments

Eryu Guan Jan. 25, 2017, 9:20 a.m. UTC | #1
On Tue, Jan 24, 2017 at 10:50:29PM -0800, Omar Sandoval wrote:
> From: Omar Sandoval <osandov@fb.com>
> 
> This is a regression test for "Btrfs: disable xattr operations on
> subvolume directories". On v4.9, it will result in an aborted
> transaction.
> 
> Signed-off-by: Omar Sandoval <osandov@fb.com>

Looks good to me overall, tested with 4.10-rc5 kernel with and without
the proposed kernel fix, results are all as expected. Some minor issues
inline.

> ---
>  tests/btrfs/047     | 69 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  tests/btrfs/047.out |  2 ++
>  tests/btrfs/group   |  1 +
>  3 files changed, 72 insertions(+)
>  create mode 100644 tests/btrfs/047

Better to add new test with mode 755, otherwise git diff shows mode
changes after tests being run.

>  create mode 100644 tests/btrfs/047.out
> 
> diff --git a/tests/btrfs/047 b/tests/btrfs/047
> new file mode 100644
> index 00000000..c3222f8b
> --- /dev/null
> +++ b/tests/btrfs/047
> @@ -0,0 +1,69 @@
> +#! /bin/bash
> +# FS QA Test 047
> +#
> +# Test that we can't set xattrs on read-only subvolume placeholder directories.
> +# Regression test for Btrfs: disable xattr operations on subvolume directories.
> +#
> +#-----------------------------------------------------------------------
> +# Copyright (c) 2017 Facebook.  All Rights Reserved.
> +#
> +# This program is free software; you can redistribute it and/or
> +# modify it under the terms of the GNU General Public License as
> +# published by the Free Software Foundation.
> +#
> +# This program is distributed in the hope that it would be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program; if not, write the Free Software Foundation,
> +# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> +#-----------------------------------------------------------------------
> +#
> +
> +seq=`basename $0`
> +seqres=$RESULT_DIR/$seq
> +echo "QA output created by $seq"
> +
> +here=`pwd`
> +tmp=/tmp/$$
> +status=1	# failure is the default!
> +trap "_cleanup; exit \$status" 0 1 2 3 15
> +
> +_cleanup()
> +{
> +	cd /
> +	rm -f $tmp.*
> +}
> +
> +# get standard environment, filters and checks
> +. ./common/rc
> +. ./common/filter
> +. ./common/attr
> +
> +# remove previous $seqres.full before test
> +rm -f $seqres.full
> +
> +# real QA test starts here
> +
> +_supported_fs btrfs
> +_supported_os Linux
> +_require_attrs
> +_require_scratch
> +
> +_scratch_mkfs >/dev/null 2>&1
> +_scratch_mount
> +
> +_run_btrfs_util_prog subvolume create "$SCRATCH_MNT/parent"
> +_run_btrfs_util_prog subvolume create "$SCRATCH_MNT/parent/child"
> +_run_btrfs_util_prog subvolume snapshot "$SCRATCH_MNT/parent" "$SCRATCH_MNT/snapshot"

Please avoid using "run_check" (called by _run_btrfs_util_prog) if
possible. We can use $BTRFS_UTIL_PROG directly and throw away stdout or
append it to $seqres.full, and stderr from these commands will break the
golden image.

Thanks,
Eryu

> +
> +$SETFATTR_PROG -n user.test -v foo "$SCRATCH_MNT/snapshot/child" |& _filter_scratch
> +
> +# The original bug resulted in bogus delayed inodes being inserted, so run the
> +# delayed inodes by doing a commit.
> +_run_btrfs_util_prog filesystem sync "$SCRATCH_MNT"
> +
> +status=0
> +exit
> diff --git a/tests/btrfs/047.out b/tests/btrfs/047.out
> new file mode 100644
> index 00000000..48555e90
> --- /dev/null
> +++ b/tests/btrfs/047.out
> @@ -0,0 +1,2 @@
> +QA output created by 047
> +setfattr: SCRATCH_MNT/snapshot/child: Operation not supported
> diff --git a/tests/btrfs/group b/tests/btrfs/group
> index 88fb8db4..69451c6b 100644
> --- a/tests/btrfs/group
> +++ b/tests/btrfs/group
> @@ -49,6 +49,7 @@
>  044 auto quick send
>  045 auto quick send
>  046 auto quick send
> +047 auto quick snapshot attr
>  048 auto quick
>  049 auto quick
>  050 auto quick send
> -- 
> 2.11.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
Qu Wenruo Jan. 25, 2017, 9:30 a.m. UTC | #2
At 01/25/2017 05:20 PM, Eryu Guan wrote:
> On Tue, Jan 24, 2017 at 10:50:29PM -0800, Omar Sandoval wrote:
>> From: Omar Sandoval <osandov@fb.com>
>>
>> This is a regression test for "Btrfs: disable xattr operations on
>> subvolume directories". On v4.9, it will result in an aborted
>> transaction.
>>
>> Signed-off-by: Omar Sandoval <osandov@fb.com>
>
> Looks good to me overall, tested with 4.10-rc5 kernel with and without
> the proposed kernel fix, results are all as expected. Some minor issues
> inline.
>
>> ---
>>  tests/btrfs/047     | 69 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  tests/btrfs/047.out |  2 ++
>>  tests/btrfs/group   |  1 +
>>  3 files changed, 72 insertions(+)
>>  create mode 100644 tests/btrfs/047
>
> Better to add new test with mode 755, otherwise git diff shows mode
> changes after tests being run.
>
>>  create mode 100644 tests/btrfs/047.out
>>
>> diff --git a/tests/btrfs/047 b/tests/btrfs/047
>> new file mode 100644
>> index 00000000..c3222f8b
>> --- /dev/null
>> +++ b/tests/btrfs/047
>> @@ -0,0 +1,69 @@
>> +#! /bin/bash
>> +# FS QA Test 047
>> +#
>> +# Test that we can't set xattrs on read-only subvolume placeholder directories.
>> +# Regression test for Btrfs: disable xattr operations on subvolume directories.
>> +#
>> +#-----------------------------------------------------------------------
>> +# Copyright (c) 2017 Facebook.  All Rights Reserved.
>> +#
>> +# This program is free software; you can redistribute it and/or
>> +# modify it under the terms of the GNU General Public License as
>> +# published by the Free Software Foundation.
>> +#
>> +# This program is distributed in the hope that it would be useful,
>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> +# GNU General Public License for more details.
>> +#
>> +# You should have received a copy of the GNU General Public License
>> +# along with this program; if not, write the Free Software Foundation,
>> +# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
>> +#-----------------------------------------------------------------------
>> +#
>> +
>> +seq=`basename $0`
>> +seqres=$RESULT_DIR/$seq
>> +echo "QA output created by $seq"
>> +
>> +here=`pwd`
>> +tmp=/tmp/$$
>> +status=1	# failure is the default!
>> +trap "_cleanup; exit \$status" 0 1 2 3 15
>> +
>> +_cleanup()
>> +{
>> +	cd /
>> +	rm -f $tmp.*
>> +}
>> +
>> +# get standard environment, filters and checks
>> +. ./common/rc
>> +. ./common/filter
>> +. ./common/attr
>> +
>> +# remove previous $seqres.full before test
>> +rm -f $seqres.full
>> +
>> +# real QA test starts here
>> +
>> +_supported_fs btrfs
>> +_supported_os Linux
>> +_require_attrs
>> +_require_scratch
>> +
>> +_scratch_mkfs >/dev/null 2>&1
>> +_scratch_mount
>> +
>> +_run_btrfs_util_prog subvolume create "$SCRATCH_MNT/parent"
>> +_run_btrfs_util_prog subvolume create "$SCRATCH_MNT/parent/child"
>> +_run_btrfs_util_prog subvolume snapshot "$SCRATCH_MNT/parent" "$SCRATCH_MNT/snapshot"
>
> Please avoid using "run_check" (called by _run_btrfs_util_prog) if
> possible. We can use $BTRFS_UTIL_PROG directly and throw away stdout or
> append it to $seqres.full, and stderr from these commands will break the
> golden image.

Isn't it designed to use "run_check" to break the test case if it 
returns any thing other than 0?

IMHO any stderr output should be unexpected in this case.

Thanks,
Qu
>
> Thanks,
> Eryu
>
>> +
>> +$SETFATTR_PROG -n user.test -v foo "$SCRATCH_MNT/snapshot/child" |& _filter_scratch
>> +
>> +# The original bug resulted in bogus delayed inodes being inserted, so run the
>> +# delayed inodes by doing a commit.
>> +_run_btrfs_util_prog filesystem sync "$SCRATCH_MNT"
>> +
>> +status=0
>> +exit
>> diff --git a/tests/btrfs/047.out b/tests/btrfs/047.out
>> new file mode 100644
>> index 00000000..48555e90
>> --- /dev/null
>> +++ b/tests/btrfs/047.out
>> @@ -0,0 +1,2 @@
>> +QA output created by 047
>> +setfattr: SCRATCH_MNT/snapshot/child: Operation not supported
>> diff --git a/tests/btrfs/group b/tests/btrfs/group
>> index 88fb8db4..69451c6b 100644
>> --- a/tests/btrfs/group
>> +++ b/tests/btrfs/group
>> @@ -49,6 +49,7 @@
>>  044 auto quick send
>>  045 auto quick send
>>  046 auto quick send
>> +047 auto quick snapshot attr
>>  048 auto quick
>>  049 auto quick
>>  050 auto quick send
>> --
>> 2.11.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
Anand Jain Jan. 25, 2017, 9:56 a.m. UTC | #3
one below.

On 01/25/17 14:50, Omar Sandoval wrote:
> From: Omar Sandoval <osandov@fb.com>
>
> This is a regression test for "Btrfs: disable xattr operations on
> subvolume directories". On v4.9, it will result in an aborted
> transaction.
>
> Signed-off-by: Omar Sandoval <osandov@fb.com>
> ---
>  tests/btrfs/047     | 69 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  tests/btrfs/047.out |  2 ++
>  tests/btrfs/group   |  1 +
>  3 files changed, 72 insertions(+)
>  create mode 100644 tests/btrfs/047
>  create mode 100644 tests/btrfs/047.out
>
> diff --git a/tests/btrfs/047 b/tests/btrfs/047
> new file mode 100644

  There is no run permission. Rest is good.


   Reviewed-by: Anand Jain <anand.jain@oracle.com>

Thanks, Anand


> index 00000000..c3222f8b
> --- /dev/null
> +++ b/tests/btrfs/047
> @@ -0,0 +1,69 @@
> +#! /bin/bash
> +# FS QA Test 047
> +#
> +# Test that we can't set xattrs on read-only subvolume placeholder directories.
> +# Regression test for Btrfs: disable xattr operations on subvolume directories.
> +#
> +#-----------------------------------------------------------------------
> +# Copyright (c) 2017 Facebook.  All Rights Reserved.
> +#
> +# This program is free software; you can redistribute it and/or
> +# modify it under the terms of the GNU General Public License as
> +# published by the Free Software Foundation.
> +#
> +# This program is distributed in the hope that it would be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program; if not, write the Free Software Foundation,
> +# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> +#-----------------------------------------------------------------------
> +#
> +
> +seq=`basename $0`
> +seqres=$RESULT_DIR/$seq
> +echo "QA output created by $seq"
> +
> +here=`pwd`
> +tmp=/tmp/$$
> +status=1	# failure is the default!
> +trap "_cleanup; exit \$status" 0 1 2 3 15
> +
> +_cleanup()
> +{
> +	cd /
> +	rm -f $tmp.*
> +}
> +
> +# get standard environment, filters and checks
> +. ./common/rc
> +. ./common/filter
> +. ./common/attr
> +
> +# remove previous $seqres.full before test
> +rm -f $seqres.full
> +
> +# real QA test starts here
> +
> +_supported_fs btrfs
> +_supported_os Linux
> +_require_attrs
> +_require_scratch
> +
> +_scratch_mkfs >/dev/null 2>&1
> +_scratch_mount
> +
> +_run_btrfs_util_prog subvolume create "$SCRATCH_MNT/parent"
> +_run_btrfs_util_prog subvolume create "$SCRATCH_MNT/parent/child"
> +_run_btrfs_util_prog subvolume snapshot "$SCRATCH_MNT/parent" "$SCRATCH_MNT/snapshot"
> +
> +$SETFATTR_PROG -n user.test -v foo "$SCRATCH_MNT/snapshot/child" |& _filter_scratch
> +
> +# The original bug resulted in bogus delayed inodes being inserted, so run the
> +# delayed inodes by doing a commit.
> +_run_btrfs_util_prog filesystem sync "$SCRATCH_MNT"
> +
> +status=0
> +exit
> diff --git a/tests/btrfs/047.out b/tests/btrfs/047.out
> new file mode 100644
> index 00000000..48555e90
> --- /dev/null
> +++ b/tests/btrfs/047.out
> @@ -0,0 +1,2 @@
> +QA output created by 047
> +setfattr: SCRATCH_MNT/snapshot/child: Operation not supported
> diff --git a/tests/btrfs/group b/tests/btrfs/group
> index 88fb8db4..69451c6b 100644
> --- a/tests/btrfs/group
> +++ b/tests/btrfs/group
> @@ -49,6 +49,7 @@
>  044 auto quick send
>  045 auto quick send
>  046 auto quick send
> +047 auto quick snapshot attr
>  048 auto quick
>  049 auto quick
>  050 auto quick send
>
--
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
Anand Jain Jan. 25, 2017, 10:02 a.m. UTC | #4
On 01/25/17 17:30, Qu Wenruo wrote:
>
>
> At 01/25/2017 05:20 PM, Eryu Guan wrote:
>> On Tue, Jan 24, 2017 at 10:50:29PM -0800, Omar Sandoval wrote:
>>> From: Omar Sandoval <osandov@fb.com>
>>>
>>> This is a regression test for "Btrfs: disable xattr operations on
>>> subvolume directories". On v4.9, it will result in an aborted
>>> transaction.
>>>
>>> Signed-off-by: Omar Sandoval <osandov@fb.com>
>>
>> Looks good to me overall, tested with 4.10-rc5 kernel with and without
>> the proposed kernel fix, results are all as expected. Some minor issues
>> inline.
>>
>>> ---
>>>  tests/btrfs/047     | 69
>>> +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>  tests/btrfs/047.out |  2 ++
>>>  tests/btrfs/group   |  1 +
>>>  3 files changed, 72 insertions(+)
>>>  create mode 100644 tests/btrfs/047
>>
>> Better to add new test with mode 755, otherwise git diff shows mode
>> changes after tests being run.
>>
>>>  create mode 100644 tests/btrfs/047.out
>>>
>>> diff --git a/tests/btrfs/047 b/tests/btrfs/047
>>> new file mode 100644
>>> index 00000000..c3222f8b
>>> --- /dev/null
>>> +++ b/tests/btrfs/047
>>> @@ -0,0 +1,69 @@
>>> +#! /bin/bash
>>> +# FS QA Test 047
>>> +#
>>> +# Test that we can't set xattrs on read-only subvolume placeholder
>>> directories.
>>> +# Regression test for Btrfs: disable xattr operations on subvolume
>>> directories.
>>> +#
>>> +#-----------------------------------------------------------------------
>>>
>>> +# Copyright (c) 2017 Facebook.  All Rights Reserved.
>>> +#
>>> +# This program is free software; you can redistribute it and/or
>>> +# modify it under the terms of the GNU General Public License as
>>> +# published by the Free Software Foundation.
>>> +#
>>> +# This program is distributed in the hope that it would be useful,
>>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>> +# GNU General Public License for more details.
>>> +#
>>> +# You should have received a copy of the GNU General Public License
>>> +# along with this program; if not, write the Free Software Foundation,
>>> +# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
>>> +#-----------------------------------------------------------------------
>>>
>>> +#
>>> +
>>> +seq=`basename $0`
>>> +seqres=$RESULT_DIR/$seq
>>> +echo "QA output created by $seq"
>>> +
>>> +here=`pwd`
>>> +tmp=/tmp/$$
>>> +status=1    # failure is the default!
>>> +trap "_cleanup; exit \$status" 0 1 2 3 15
>>> +
>>> +_cleanup()
>>> +{
>>> +    cd /
>>> +    rm -f $tmp.*
>>> +}
>>> +
>>> +# get standard environment, filters and checks
>>> +. ./common/rc
>>> +. ./common/filter
>>> +. ./common/attr
>>> +
>>> +# remove previous $seqres.full before test
>>> +rm -f $seqres.full
>>> +
>>> +# real QA test starts here
>>> +
>>> +_supported_fs btrfs
>>> +_supported_os Linux
>>> +_require_attrs
>>> +_require_scratch
>>> +
>>> +_scratch_mkfs >/dev/null 2>&1
>>> +_scratch_mount
>>> +
>>> +_run_btrfs_util_prog subvolume create "$SCRATCH_MNT/parent"
>>> +_run_btrfs_util_prog subvolume create "$SCRATCH_MNT/parent/child"
>>> +_run_btrfs_util_prog subvolume snapshot "$SCRATCH_MNT/parent"
>>> "$SCRATCH_MNT/snapshot"
>>
>> Please avoid using "run_check" (called by _run_btrfs_util_prog) if
>> possible. We can use $BTRFS_UTIL_PROG directly and throw away stdout or
>> append it to $seqres.full, and stderr from these commands will break the
>> golden image.
>
> Isn't it designed to use "run_check" to break the test case if it
> returns any thing other than 0?
>
> IMHO any stderr output should be unexpected in this case.

  I agree too. Its was mentioned before as well, we should rather
  check the return code instead of strings printed on cli.

Thanks, Anand

> Thanks,
> Qu
>>
>> Thanks,
>> Eryu
>>
>>> +
>>> +$SETFATTR_PROG -n user.test -v foo "$SCRATCH_MNT/snapshot/child" |&
>>> _filter_scratch
>>> +
>>> +# The original bug resulted in bogus delayed inodes being inserted,
>>> so run the
>>> +# delayed inodes by doing a commit.
>>> +_run_btrfs_util_prog filesystem sync "$SCRATCH_MNT"
>>> +
>>> +status=0
>>> +exit
>>> diff --git a/tests/btrfs/047.out b/tests/btrfs/047.out
>>> new file mode 100644
>>> index 00000000..48555e90
>>> --- /dev/null
>>> +++ b/tests/btrfs/047.out
>>> @@ -0,0 +1,2 @@
>>> +QA output created by 047
>>> +setfattr: SCRATCH_MNT/snapshot/child: Operation not supported
>>> diff --git a/tests/btrfs/group b/tests/btrfs/group
>>> index 88fb8db4..69451c6b 100644
>>> --- a/tests/btrfs/group
>>> +++ b/tests/btrfs/group
>>> @@ -49,6 +49,7 @@
>>>  044 auto quick send
>>>  045 auto quick send
>>>  046 auto quick send
>>> +047 auto quick snapshot attr
>>>  048 auto quick
>>>  049 auto quick
>>>  050 auto quick send
>>> --
>>> 2.11.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 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
Eryu Guan Jan. 25, 2017, 10:24 a.m. UTC | #5
On Wed, Jan 25, 2017 at 05:30:32PM +0800, Qu Wenruo wrote:
> 
> 
> At 01/25/2017 05:20 PM, Eryu Guan wrote:
> > On Tue, Jan 24, 2017 at 10:50:29PM -0800, Omar Sandoval wrote:
> > > From: Omar Sandoval <osandov@fb.com>
> > > 
> > > This is a regression test for "Btrfs: disable xattr operations on
> > > subvolume directories". On v4.9, it will result in an aborted
> > > transaction.
> > > 
> > > Signed-off-by: Omar Sandoval <osandov@fb.com>
> > 
> > Looks good to me overall, tested with 4.10-rc5 kernel with and without
> > the proposed kernel fix, results are all as expected. Some minor issues
> > inline.
> > 
> > > ---
> > >  tests/btrfs/047     | 69 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  tests/btrfs/047.out |  2 ++
> > >  tests/btrfs/group   |  1 +
> > >  3 files changed, 72 insertions(+)
> > >  create mode 100644 tests/btrfs/047
> > 
> > Better to add new test with mode 755, otherwise git diff shows mode
> > changes after tests being run.
> > 
> > >  create mode 100644 tests/btrfs/047.out
> > > 
> > > diff --git a/tests/btrfs/047 b/tests/btrfs/047
> > > new file mode 100644
> > > index 00000000..c3222f8b
> > > --- /dev/null
> > > +++ b/tests/btrfs/047
> > > @@ -0,0 +1,69 @@
> > > +#! /bin/bash
> > > +# FS QA Test 047
> > > +#
> > > +# Test that we can't set xattrs on read-only subvolume placeholder directories.
> > > +# Regression test for Btrfs: disable xattr operations on subvolume directories.
> > > +#
> > > +#-----------------------------------------------------------------------
> > > +# Copyright (c) 2017 Facebook.  All Rights Reserved.
> > > +#
> > > +# This program is free software; you can redistribute it and/or
> > > +# modify it under the terms of the GNU General Public License as
> > > +# published by the Free Software Foundation.
> > > +#
> > > +# This program is distributed in the hope that it would be useful,
> > > +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > > +# GNU General Public License for more details.
> > > +#
> > > +# You should have received a copy of the GNU General Public License
> > > +# along with this program; if not, write the Free Software Foundation,
> > > +# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> > > +#-----------------------------------------------------------------------
> > > +#
> > > +
> > > +seq=`basename $0`
> > > +seqres=$RESULT_DIR/$seq
> > > +echo "QA output created by $seq"
> > > +
> > > +here=`pwd`
> > > +tmp=/tmp/$$
> > > +status=1	# failure is the default!
> > > +trap "_cleanup; exit \$status" 0 1 2 3 15
> > > +
> > > +_cleanup()
> > > +{
> > > +	cd /
> > > +	rm -f $tmp.*
> > > +}
> > > +
> > > +# get standard environment, filters and checks
> > > +. ./common/rc
> > > +. ./common/filter
> > > +. ./common/attr
> > > +
> > > +# remove previous $seqres.full before test
> > > +rm -f $seqres.full
> > > +
> > > +# real QA test starts here
> > > +
> > > +_supported_fs btrfs
> > > +_supported_os Linux
> > > +_require_attrs
> > > +_require_scratch
> > > +
> > > +_scratch_mkfs >/dev/null 2>&1
> > > +_scratch_mount
> > > +
> > > +_run_btrfs_util_prog subvolume create "$SCRATCH_MNT/parent"
> > > +_run_btrfs_util_prog subvolume create "$SCRATCH_MNT/parent/child"
> > > +_run_btrfs_util_prog subvolume snapshot "$SCRATCH_MNT/parent" "$SCRATCH_MNT/snapshot"
> > 
> > Please avoid using "run_check" (called by _run_btrfs_util_prog) if
> > possible. We can use $BTRFS_UTIL_PROG directly and throw away stdout or
> > append it to $seqres.full, and stderr from these commands will break the
> > golden image.
> 
> Isn't it designed to use "run_check" to break the test case if it returns
> any thing other than 0?

The whole fstests is built upon the golden image matching mechanism,
it's designed to let you don't have to check the return value of
commands. IMO, this run_check should not be introduced in the first
place. I agree that this "golden image matching" has some problems, e.g.
we have to write lots of filters to make the outputs deterministic, it
has both up and down sides, as well as the "check return value" way. But
that's the way fstests has chosen to go since the beginning.

> 
> IMHO any stderr output should be unexpected in this case.

Yes, so any unexpected output breaks & fails the test, and we can see
why it fails from the diff output. And run_check "hides" this info and
makes it harder to parse the result, e.g.

-setfattr: SCRATCH_MNT/snapshot/child: Operation not supported
+failed: '/usr/local/bin/btrfs filesystem sync /mnt/testarea/scratch'
+(see /root/workspace/xfstests/results//btrfs/btrfs/047.full for details)

And without run_check:

-setfattr: SCRATCH_MNT/snapshot/child: Operation not supported
+ERROR: sync ioctl failed on '/mnt/testarea/scratch': No such file or directory

We know why and where it fails.

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
Qu Wenruo Jan. 26, 2017, 12:31 a.m. UTC | #6
At 01/25/2017 06:24 PM, Eryu Guan wrote:
> On Wed, Jan 25, 2017 at 05:30:32PM +0800, Qu Wenruo wrote:
>>
>>
>> At 01/25/2017 05:20 PM, Eryu Guan wrote:
>>> On Tue, Jan 24, 2017 at 10:50:29PM -0800, Omar Sandoval wrote:
>>>> From: Omar Sandoval <osandov@fb.com>
>>>>
>>>> This is a regression test for "Btrfs: disable xattr operations on
>>>> subvolume directories". On v4.9, it will result in an aborted
>>>> transaction.
>>>>
>>>> Signed-off-by: Omar Sandoval <osandov@fb.com>
>>>
>>> Looks good to me overall, tested with 4.10-rc5 kernel with and without
>>> the proposed kernel fix, results are all as expected. Some minor issues
>>> inline.
>>>
>>>> ---
>>>>  tests/btrfs/047     | 69 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>  tests/btrfs/047.out |  2 ++
>>>>  tests/btrfs/group   |  1 +
>>>>  3 files changed, 72 insertions(+)
>>>>  create mode 100644 tests/btrfs/047
>>>
>>> Better to add new test with mode 755, otherwise git diff shows mode
>>> changes after tests being run.
>>>
>>>>  create mode 100644 tests/btrfs/047.out
>>>>
>>>> diff --git a/tests/btrfs/047 b/tests/btrfs/047
>>>> new file mode 100644
>>>> index 00000000..c3222f8b
>>>> --- /dev/null
>>>> +++ b/tests/btrfs/047
>>>> @@ -0,0 +1,69 @@
>>>> +#! /bin/bash
>>>> +# FS QA Test 047
>>>> +#
>>>> +# Test that we can't set xattrs on read-only subvolume placeholder directories.
>>>> +# Regression test for Btrfs: disable xattr operations on subvolume directories.
>>>> +#
>>>> +#-----------------------------------------------------------------------
>>>> +# Copyright (c) 2017 Facebook.  All Rights Reserved.
>>>> +#
>>>> +# This program is free software; you can redistribute it and/or
>>>> +# modify it under the terms of the GNU General Public License as
>>>> +# published by the Free Software Foundation.
>>>> +#
>>>> +# This program is distributed in the hope that it would be useful,
>>>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>>>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>>> +# GNU General Public License for more details.
>>>> +#
>>>> +# You should have received a copy of the GNU General Public License
>>>> +# along with this program; if not, write the Free Software Foundation,
>>>> +# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
>>>> +#-----------------------------------------------------------------------
>>>> +#
>>>> +
>>>> +seq=`basename $0`
>>>> +seqres=$RESULT_DIR/$seq
>>>> +echo "QA output created by $seq"
>>>> +
>>>> +here=`pwd`
>>>> +tmp=/tmp/$$
>>>> +status=1	# failure is the default!
>>>> +trap "_cleanup; exit \$status" 0 1 2 3 15
>>>> +
>>>> +_cleanup()
>>>> +{
>>>> +	cd /
>>>> +	rm -f $tmp.*
>>>> +}
>>>> +
>>>> +# get standard environment, filters and checks
>>>> +. ./common/rc
>>>> +. ./common/filter
>>>> +. ./common/attr
>>>> +
>>>> +# remove previous $seqres.full before test
>>>> +rm -f $seqres.full
>>>> +
>>>> +# real QA test starts here
>>>> +
>>>> +_supported_fs btrfs
>>>> +_supported_os Linux
>>>> +_require_attrs
>>>> +_require_scratch
>>>> +
>>>> +_scratch_mkfs >/dev/null 2>&1
>>>> +_scratch_mount
>>>> +
>>>> +_run_btrfs_util_prog subvolume create "$SCRATCH_MNT/parent"
>>>> +_run_btrfs_util_prog subvolume create "$SCRATCH_MNT/parent/child"
>>>> +_run_btrfs_util_prog subvolume snapshot "$SCRATCH_MNT/parent" "$SCRATCH_MNT/snapshot"
>>>
>>> Please avoid using "run_check" (called by _run_btrfs_util_prog) if
>>> possible. We can use $BTRFS_UTIL_PROG directly and throw away stdout or
>>> append it to $seqres.full, and stderr from these commands will break the
>>> golden image.
>>
>> Isn't it designed to use "run_check" to break the test case if it returns
>> any thing other than 0?
>
> The whole fstests is built upon the golden image matching mechanism,
> it's designed to let you don't have to check the return value of
> commands. IMO, this run_check should not be introduced in the first
> place. I agree that this "golden image matching" has some problems, e.g.
> we have to write lots of filters to make the outputs deterministic, it
> has both up and down sides, as well as the "check return value" way. But
> that's the way fstests has chosen to go since the beginning.

However run_check makes the test exits early, which is quite handy for 
stress test.

>
>>
>> IMHO any stderr output should be unexpected in this case.
>
> Yes, so any unexpected output breaks & fails the test, and we can see
> why it fails from the diff output. And run_check "hides" this info and
> makes it harder to parse the result, e.g.
>
> -setfattr: SCRATCH_MNT/snapshot/child: Operation not supported
> +failed: '/usr/local/bin/btrfs filesystem sync /mnt/testarea/scratch'
> +(see /root/workspace/xfstests/results//btrfs/btrfs/047.full for details)
>
> And without run_check:
>
> -setfattr: SCRATCH_MNT/snapshot/child: Operation not supported
> +ERROR: sync ioctl failed on '/mnt/testarea/scratch': No such file or directory
>
> We know why and where it fails.

True, it makes the output more user friendly.

However IMHO it's a problem of current run_check, why not to modify 
run_check to redirect stderr to stdout?

Then everyone should be happy.

Thanks,
Qu

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

Patch

diff --git a/tests/btrfs/047 b/tests/btrfs/047
new file mode 100644
index 00000000..c3222f8b
--- /dev/null
+++ b/tests/btrfs/047
@@ -0,0 +1,69 @@ 
+#! /bin/bash
+# FS QA Test 047
+#
+# Test that we can't set xattrs on read-only subvolume placeholder directories.
+# Regression test for Btrfs: disable xattr operations on subvolume directories.
+#
+#-----------------------------------------------------------------------
+# Copyright (c) 2017 Facebook.  All Rights Reserved.
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License as
+# published by the Free Software Foundation.
+#
+# This program is distributed in the hope that it would be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write the Free Software Foundation,
+# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+#-----------------------------------------------------------------------
+#
+
+seq=`basename $0`
+seqres=$RESULT_DIR/$seq
+echo "QA output created by $seq"
+
+here=`pwd`
+tmp=/tmp/$$
+status=1	# failure is the default!
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+_cleanup()
+{
+	cd /
+	rm -f $tmp.*
+}
+
+# get standard environment, filters and checks
+. ./common/rc
+. ./common/filter
+. ./common/attr
+
+# remove previous $seqres.full before test
+rm -f $seqres.full
+
+# real QA test starts here
+
+_supported_fs btrfs
+_supported_os Linux
+_require_attrs
+_require_scratch
+
+_scratch_mkfs >/dev/null 2>&1
+_scratch_mount
+
+_run_btrfs_util_prog subvolume create "$SCRATCH_MNT/parent"
+_run_btrfs_util_prog subvolume create "$SCRATCH_MNT/parent/child"
+_run_btrfs_util_prog subvolume snapshot "$SCRATCH_MNT/parent" "$SCRATCH_MNT/snapshot"
+
+$SETFATTR_PROG -n user.test -v foo "$SCRATCH_MNT/snapshot/child" |& _filter_scratch
+
+# The original bug resulted in bogus delayed inodes being inserted, so run the
+# delayed inodes by doing a commit.
+_run_btrfs_util_prog filesystem sync "$SCRATCH_MNT"
+
+status=0
+exit
diff --git a/tests/btrfs/047.out b/tests/btrfs/047.out
new file mode 100644
index 00000000..48555e90
--- /dev/null
+++ b/tests/btrfs/047.out
@@ -0,0 +1,2 @@ 
+QA output created by 047
+setfattr: SCRATCH_MNT/snapshot/child: Operation not supported
diff --git a/tests/btrfs/group b/tests/btrfs/group
index 88fb8db4..69451c6b 100644
--- a/tests/btrfs/group
+++ b/tests/btrfs/group
@@ -49,6 +49,7 @@ 
 044 auto quick send
 045 auto quick send
 046 auto quick send
+047 auto quick snapshot attr
 048 auto quick
 049 auto quick
 050 auto quick send