* Re: [PATCH v3] common/encrypt, ceph: add _require_not_encrypted test
2021-12-15 19:02 [PATCH v3] common/encrypt, ceph: add _require_not_encrypted test Jeff Layton
@ 2021-12-16 0:45 ` Eric Biggers
2021-12-16 10:04 ` Luís Henriques
2021-12-19 13:20 ` Eryu Guan
2 siblings, 0 replies; 4+ messages in thread
From: Eric Biggers @ 2021-12-16 0:45 UTC (permalink / raw)
To: Jeff Layton; +Cc: fstests, Luis Henriques
On Wed, Dec 15, 2021 at 02:02:48PM -0500, Jeff Layton wrote:
> Some tests on ceph require changing the layout of new files, which is
> forbidden when the files are encrypted. Add a test that touches a file
> in $TEST_DIR and then tests it to see if it reports the
> STATX_ATTR_ENCRYPTED flag, and does a _notrun if it's present.
>
> Also add this requirement to three ceph tests that change the layout.
>
> Cc: Luis Henriques <lhenriques@suse.de>
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
> common/encrypt | 18 ++++++++++++++++++
> tests/ceph/001 | 2 ++
> tests/ceph/002 | 2 ++
> tests/ceph/003 | 2 ++
> 4 files changed, 24 insertions(+)
>
> v2: make ceph/001 also call _require_not_encrypted
>
> v3: move test into common/encrypt
>
> diff --git a/common/encrypt b/common/encrypt
> index f90c4ef05a3f..156425af40a8 100644
> --- a/common/encrypt
> +++ b/common/encrypt
> @@ -940,3 +940,21 @@ _filter_nokey_filenames()
> # of characters that have ever been used in such names.
> sed "s|${dir}${dir:+/}[A-Za-z0-9+,_-]\+|${dir}${dir:+/}NOKEY_NAME|g"
> }
> +
> +# Some tests require that encryption not be enabled.
> +_require_not_encrypted()
> +{
> + local target=$TEST_DIR/require_not_encrypted.$$
> + local ret=0
> +
> + #
> + # The top-level directory mounted with test_dummy_encryption is not
> + # itself encrypted. Only new files and directories created under it
> + # are.
> + touch $target
> + local attrs=$($XFS_IO_PROG -c 'statx -r' $target | awk '/stat.attributes / { print $3 }')
> + rm -f $target
> +
> + # STATX_ATTR_ENCRYPTED == 0x800
> + [ $(( attrs & 0x800 )) -eq 0 ] || _notrun "Filesystem is encrypted"
> +}
Did you consider instead skipping the test if the mount options contain
test_dummy_encryption? That's the way that the encryption tests currently
detect that they shouldn't run (since the automatic encryption caused by
test_dummy_encryption would interfere with them); see
_require_scratch_encryption(). What you've done here will work too, but it
might make sense to check for test_dummy_encryption the same way in both places.
- Eric
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH v3] common/encrypt, ceph: add _require_not_encrypted test
2021-12-15 19:02 [PATCH v3] common/encrypt, ceph: add _require_not_encrypted test Jeff Layton
2021-12-16 0:45 ` Eric Biggers
@ 2021-12-16 10:04 ` Luís Henriques
2021-12-19 13:20 ` Eryu Guan
2 siblings, 0 replies; 4+ messages in thread
From: Luís Henriques @ 2021-12-16 10:04 UTC (permalink / raw)
To: Jeff Layton; +Cc: fstests
On Wed, Dec 15, 2021 at 02:02:48PM -0500, Jeff Layton wrote:
> Some tests on ceph require changing the layout of new files, which is
> forbidden when the files are encrypted. Add a test that touches a file
> in $TEST_DIR and then tests it to see if it reports the
> STATX_ATTR_ENCRYPTED flag, and does a _notrun if it's present.
>
> Also add this requirement to three ceph tests that change the layout.
>
> Cc: Luis Henriques <lhenriques@suse.de>
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
> common/encrypt | 18 ++++++++++++++++++
> tests/ceph/001 | 2 ++
> tests/ceph/002 | 2 ++
> tests/ceph/003 | 2 ++
> 4 files changed, 24 insertions(+)
>
> v2: make ceph/001 also call _require_not_encrypted
>
> v3: move test into common/encrypt
Awesome, looks good to me. Feel free to add my
Reviewed-by: Luis Henriques <lhenriques@suse.de>
(Although you may send another revision with Eric's suggestion...)
Cheers,
--
Luís
> diff --git a/common/encrypt b/common/encrypt
> index f90c4ef05a3f..156425af40a8 100644
> --- a/common/encrypt
> +++ b/common/encrypt
> @@ -940,3 +940,21 @@ _filter_nokey_filenames()
> # of characters that have ever been used in such names.
> sed "s|${dir}${dir:+/}[A-Za-z0-9+,_-]\+|${dir}${dir:+/}NOKEY_NAME|g"
> }
> +
> +# Some tests require that encryption not be enabled.
> +_require_not_encrypted()
> +{
> + local target=$TEST_DIR/require_not_encrypted.$$
> + local ret=0
> +
> + #
> + # The top-level directory mounted with test_dummy_encryption is not
> + # itself encrypted. Only new files and directories created under it
> + # are.
> + touch $target
> + local attrs=$($XFS_IO_PROG -c 'statx -r' $target | awk '/stat.attributes / { print $3 }')
> + rm -f $target
> +
> + # STATX_ATTR_ENCRYPTED == 0x800
> + [ $(( attrs & 0x800 )) -eq 0 ] || _notrun "Filesystem is encrypted"
> +}
> diff --git a/tests/ceph/001 b/tests/ceph/001
> index c00de308fd95..d6ded026a6dd 100755
> --- a/tests/ceph/001
> +++ b/tests/ceph/001
> @@ -17,11 +17,13 @@ _begin_fstest auto quick copy_range
> . common/filter
> . common/attr
> . common/reflink
> +. common/encrypt
>
> # real QA test starts here
> _supported_fs ceph
>
> _require_xfs_io_command "copy_range"
> +_require_not_encrypted
> _require_attrs
> _require_test
>
> diff --git a/tests/ceph/002 b/tests/ceph/002
> index 9bc728fd2e18..2232b75e6dfa 100755
> --- a/tests/ceph/002
> +++ b/tests/ceph/002
> @@ -25,11 +25,13 @@ _begin_fstest auto quick copy_range
> # get standard environment
> . common/filter
> . common/attr
> +. common/encrypt
>
> # real QA test starts here
> _supported_fs ceph
>
> _require_xfs_io_command "copy_range"
> +_require_not_encrypted
> _require_attrs
> _require_test
>
> diff --git a/tests/ceph/003 b/tests/ceph/003
> index faedb48cfeea..aa130ae807f9 100755
> --- a/tests/ceph/003
> +++ b/tests/ceph/003
> @@ -13,11 +13,13 @@ _begin_fstest auto quick copy_range
> . common/filter
> . common/attr
> . common/reflink
> +. common/encrypt
>
> # real QA test starts here
> _supported_fs ceph
>
> _require_xfs_io_command "copy_range"
> +_require_not_encrypted
> _require_attrs
> _require_test
>
> --
> 2.33.1
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH v3] common/encrypt, ceph: add _require_not_encrypted test
2021-12-15 19:02 [PATCH v3] common/encrypt, ceph: add _require_not_encrypted test Jeff Layton
2021-12-16 0:45 ` Eric Biggers
2021-12-16 10:04 ` Luís Henriques
@ 2021-12-19 13:20 ` Eryu Guan
2 siblings, 0 replies; 4+ messages in thread
From: Eryu Guan @ 2021-12-19 13:20 UTC (permalink / raw)
To: Jeff Layton; +Cc: fstests, Luis Henriques
On Wed, Dec 15, 2021 at 02:02:48PM -0500, Jeff Layton wrote:
> Some tests on ceph require changing the layout of new files, which is
> forbidden when the files are encrypted. Add a test that touches a file
> in $TEST_DIR and then tests it to see if it reports the
> STATX_ATTR_ENCRYPTED flag, and does a _notrun if it's present.
>
> Also add this requirement to three ceph tests that change the layout.
>
> Cc: Luis Henriques <lhenriques@suse.de>
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
> common/encrypt | 18 ++++++++++++++++++
> tests/ceph/001 | 2 ++
> tests/ceph/002 | 2 ++
> tests/ceph/003 | 2 ++
> 4 files changed, 24 insertions(+)
>
> v2: make ceph/001 also call _require_not_encrypted
>
> v3: move test into common/encrypt
>
> diff --git a/common/encrypt b/common/encrypt
> index f90c4ef05a3f..156425af40a8 100644
> --- a/common/encrypt
> +++ b/common/encrypt
> @@ -940,3 +940,21 @@ _filter_nokey_filenames()
> # of characters that have ever been used in such names.
> sed "s|${dir}${dir:+/}[A-Za-z0-9+,_-]\+|${dir}${dir:+/}NOKEY_NAME|g"
> }
> +
> +# Some tests require that encryption not be enabled.
> +_require_not_encrypted()
We have some other helpers that require something not enabled or present,
and they're all in the format as:
_require_no_compress()
_require_no_realtime()
_require_no_xfs_debug()
So how about name it as _require_no_encryption() to be consistent with
other functions?
> +{
> + local target=$TEST_DIR/require_not_encrypted.$$
> + local ret=0
> +
> + #
> + # The top-level directory mounted with test_dummy_encryption is not
> + # itself encrypted. Only new files and directories created under it
> + # are.
> + touch $target
> + local attrs=$($XFS_IO_PROG -c 'statx -r' $target | awk '/stat.attributes / { print $3 }')
I think we may need to check if statx is supported as well
_require_xfs_io_command "statx" "-r"
Thanks,
Eryu
> + rm -f $target
> +
> + # STATX_ATTR_ENCRYPTED == 0x800
> + [ $(( attrs & 0x800 )) -eq 0 ] || _notrun "Filesystem is encrypted"
> +}
> diff --git a/tests/ceph/001 b/tests/ceph/001
> index c00de308fd95..d6ded026a6dd 100755
> --- a/tests/ceph/001
> +++ b/tests/ceph/001
> @@ -17,11 +17,13 @@ _begin_fstest auto quick copy_range
> . common/filter
> . common/attr
> . common/reflink
> +. common/encrypt
>
> # real QA test starts here
> _supported_fs ceph
>
> _require_xfs_io_command "copy_range"
> +_require_not_encrypted
> _require_attrs
> _require_test
>
> diff --git a/tests/ceph/002 b/tests/ceph/002
> index 9bc728fd2e18..2232b75e6dfa 100755
> --- a/tests/ceph/002
> +++ b/tests/ceph/002
> @@ -25,11 +25,13 @@ _begin_fstest auto quick copy_range
> # get standard environment
> . common/filter
> . common/attr
> +. common/encrypt
>
> # real QA test starts here
> _supported_fs ceph
>
> _require_xfs_io_command "copy_range"
> +_require_not_encrypted
> _require_attrs
> _require_test
>
> diff --git a/tests/ceph/003 b/tests/ceph/003
> index faedb48cfeea..aa130ae807f9 100755
> --- a/tests/ceph/003
> +++ b/tests/ceph/003
> @@ -13,11 +13,13 @@ _begin_fstest auto quick copy_range
> . common/filter
> . common/attr
> . common/reflink
> +. common/encrypt
>
> # real QA test starts here
> _supported_fs ceph
>
> _require_xfs_io_command "copy_range"
> +_require_not_encrypted
> _require_attrs
> _require_test
>
> --
> 2.33.1
^ permalink raw reply [flat|nested] 4+ messages in thread