* [PATCH 1/2] Add support for UBIFS
2017-05-17 9:55 Add UBIFS support to xfstests David Oberhollenzer
@ 2017-05-17 9:55 ` David Oberhollenzer
2017-05-17 11:53 ` Eryu Guan
2017-05-17 9:55 ` [PATCH 2/2] Accept failing with EPERM in addition to ENOKEY for rename without key David Oberhollenzer
` (2 subsequent siblings)
3 siblings, 1 reply; 10+ messages in thread
From: David Oberhollenzer @ 2017-05-17 9:55 UTC (permalink / raw)
To: fstests; +Cc: linux-mtd, richard, David Oberhollenzer
This patch is mostly based on the previous attempts of Eric Biggers
and Dongsheng Yang at adding UBIFS support to xfstests.
In addition to rebasing the previous attempts to recent xfstests,
most of the encryption tests now also support UBIFS with this patch.
Since mkfs.ubifs doesn't support creating encryption-capable filesystems
yet, _scratch_mkfs_encrypted() is modified to wipe the underlying UBI
volume. The filesystem is then created when mounting the emtpy volume.
Some tests that require SCRATCH_DEV to be a block device, despite
not using device-mapper or otherwise doing something block device
specific have that requirement removed.
Signed-off-by: David Oberhollenzer <david.oberhollenzer@sigma-star.at>
---
check | 2 ++
common/config | 7 +++++++
common/encrypt | 3 +++
common/rc | 24 ++++++++++++++++++++++++
tests/generic/076 | 1 -
tests/generic/409 | 1 -
tests/generic/410 | 1 -
tests/generic/411 | 1 -
8 files changed, 36 insertions(+), 4 deletions(-)
diff --git a/check b/check
index 9cef58b4..f8db3cd6 100755
--- a/check
+++ b/check
@@ -70,6 +70,7 @@ check options
-overlay test overlay
-pvfs2 test PVFS2
-tmpfs test TMPFS
+ -ubifs test ubifs
-l line mode diff
-udiff show unified diff (default)
-n show me, do not run tests
@@ -267,6 +268,7 @@ while [ $# -gt 0 ]; do
-overlay) FSTYP=overlay; export OVERLAY=true ;;
-pvfs2) FSTYP=pvfs2 ;;
-tmpfs) FSTYP=tmpfs ;;
+ -ubifs) FSTYP=ubifs ;;
-g) group=$2 ; shift ;
GROUP_LIST="$GROUP_LIST ${group//,/ }"
diff --git a/common/config b/common/config
index 59041a39..6c58e888 100644
--- a/common/config
+++ b/common/config
@@ -336,6 +336,9 @@ _mount_opts()
# We need to specify the size at mount, use 1G by default
export MOUNT_OPTIONS="-o size=1G $TMPFS_MOUNT_OPTIONS"
;;
+ ubifs)
+ export MOUNT_OPTIONS=$UBIFS_MOUNT_OPTIONS
+ ;;
*)
;;
esac
@@ -475,6 +478,10 @@ _check_device()
if [ ! -d "$dev" ]; then
_fatal "common/config: $name ($dev) is not a directory for overlay"
fi
+ elif [ "$FSTYP" == "ubifs" ]; then
+ if [ ! -c "$dev" ]; then
+ _fatal "common/config: $name ($dev) is not a character device"
+ fi
else
_fatal "common/config: $name ($dev) is not a block device or a network filesystem"
fi
diff --git a/common/encrypt b/common/encrypt
index 723f1b11..b444c82d 100644
--- a/common/encrypt
+++ b/common/encrypt
@@ -71,6 +71,9 @@ _scratch_mkfs_encrypted()
ext4|f2fs)
_scratch_mkfs -O encrypt
;;
+ ubifs)
+ ubiupdatevol ${SCRATCH_DEV} -t
+ ;;
*)
_notrun "No encryption support for $FSTYP"
;;
diff --git a/common/rc b/common/rc
index 257b1903..6191a0c0 100644
--- a/common/rc
+++ b/common/rc
@@ -1556,6 +1556,15 @@ _require_scratch_nocheck()
_notrun "this test requires a valid \$SCRATCH_MNT and unique $SCRATCH_DEV"
fi
;;
+ ubifs)
+ # ubifs needs an UBI volume. This will be a char device, not a block device.
+ if [ ! -c "$SCRATCH_DEV" ]; then
+ _notrun "this test requires a valid UBI volume for \$SCRATCH_DEV"
+ fi
+ if [ ! -d "$SCRATCH_MNT" ]; then
+ _notrun "this test requires a valid \$SCRATCH_MNT"
+ fi
+ ;;
*)
if [ -z "$SCRATCH_DEV" -o "`_is_block_dev "$SCRATCH_DEV"`" = "" ]
then
@@ -1650,6 +1659,15 @@ _require_test()
_notrun "this test requires a valid \$TEST_DIR and unique $TEST_DEV"
fi
;;
+ ubifs)
+ # ubifs needs an UBI volume. This will be a char device, not a block device.
+ if [ ! -c "$TEST_DEV" ]; then
+ _notrun "this test requires a valid UBI volume for \$TEST_DEV"
+ fi
+ if [ ! -d "$TEST_DIR" ]; then
+ _notrun "this test requires a valid \$TEST_DIR"
+ fi
+ ;;
*)
if [ -z "$TEST_DEV" ] || [ "`_is_block_dev "$TEST_DEV"`" = "" ]
then
@@ -2490,6 +2508,9 @@ _check_test_fs()
tmpfs)
# no way to check consistency for tmpfs
;;
+ ubifs)
+ # there is no fsck program for ubifs yet
+ ;;
*)
_check_generic_filesystem $TEST_DEV
;;
@@ -2539,6 +2560,9 @@ _check_scratch_fs()
tmpfs)
# no way to check consistency for tmpfs
;;
+ ubifs)
+ # there is no fsck program for ubifs yet
+ ;;
*)
_check_generic_filesystem $device
;;
diff --git a/tests/generic/076 b/tests/generic/076
index 64e69583..53e56a69 100755
--- a/tests/generic/076
+++ b/tests/generic/076
@@ -56,7 +56,6 @@ _supported_fs generic
_supported_os IRIX Linux
_require_scratch
-_require_block_device $SCRATCH_DEV
echo "*** init fs"
diff --git a/tests/generic/409 b/tests/generic/409
index 4bfedf73..3be58ba5 100755
--- a/tests/generic/409
+++ b/tests/generic/409
@@ -64,7 +64,6 @@ _supported_fs generic
_supported_os Linux
_require_test
_require_scratch
-_require_block_device $SCRATCH_DEV
fs_stress()
{
diff --git a/tests/generic/410 b/tests/generic/410
index f2e0d1bb..e3a933ce 100755
--- a/tests/generic/410
+++ b/tests/generic/410
@@ -72,7 +72,6 @@ _supported_fs generic
_supported_os Linux
_require_test
_require_scratch
-_require_block_device $SCRATCH_DEV
fs_stress()
{
diff --git a/tests/generic/411 b/tests/generic/411
index 7b2dd33b..08b861c5 100755
--- a/tests/generic/411
+++ b/tests/generic/411
@@ -53,7 +53,6 @@ _supported_fs generic
_supported_os Linux
_require_test
_require_scratch
-_require_block_device $SCRATCH_DEV
fs_stress()
{
--
2.12.0
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH 1/2] Add support for UBIFS
2017-05-17 9:55 ` [PATCH 1/2] Add support for UBIFS David Oberhollenzer
@ 2017-05-17 11:53 ` Eryu Guan
2017-05-17 18:45 ` Eric Biggers
0 siblings, 1 reply; 10+ messages in thread
From: Eryu Guan @ 2017-05-17 11:53 UTC (permalink / raw)
To: David Oberhollenzer; +Cc: fstests, linux-mtd, richard
On Wed, May 17, 2017 at 11:55:29AM +0200, David Oberhollenzer wrote:
> This patch is mostly based on the previous attempts of Eric Biggers
> and Dongsheng Yang at adding UBIFS support to xfstests.
>
> In addition to rebasing the previous attempts to recent xfstests,
> most of the encryption tests now also support UBIFS with this patch.
>
> Since mkfs.ubifs doesn't support creating encryption-capable filesystems
> yet, _scratch_mkfs_encrypted() is modified to wipe the underlying UBI
> volume. The filesystem is then created when mounting the emtpy volume.
>
> Some tests that require SCRATCH_DEV to be a block device, despite
> not using device-mapper or otherwise doing something block device
> specific have that requirement removed.
>
> Signed-off-by: David Oberhollenzer <david.oberhollenzer@sigma-star.at>
Thanks for the patch! I know only very little about ubifs (from previous
reviews to attempts to add ubifs support to fstests), but it's still
good to have some words to introduce ubifs briefly in commit log,
especially the fact that it uses char device not block device.
> ---
> check | 2 ++
> common/config | 7 +++++++
> common/encrypt | 3 +++
> common/rc | 24 ++++++++++++++++++++++++
> tests/generic/076 | 1 -
> tests/generic/409 | 1 -
> tests/generic/410 | 1 -
> tests/generic/411 | 1 -
> 8 files changed, 36 insertions(+), 4 deletions(-)
>
> diff --git a/check b/check
> index 9cef58b4..f8db3cd6 100755
> --- a/check
> +++ b/check
> @@ -70,6 +70,7 @@ check options
> -overlay test overlay
> -pvfs2 test PVFS2
> -tmpfs test TMPFS
> + -ubifs test ubifs
> -l line mode diff
> -udiff show unified diff (default)
> -n show me, do not run tests
> @@ -267,6 +268,7 @@ while [ $# -gt 0 ]; do
> -overlay) FSTYP=overlay; export OVERLAY=true ;;
> -pvfs2) FSTYP=pvfs2 ;;
> -tmpfs) FSTYP=tmpfs ;;
> + -ubifs) FSTYP=ubifs ;;
As being pointed out in previous reviews, it'll be great if we can probe
ubifs from the char device if possible instead of adding new fs-specific
option, just as what we're doing at the end of common/config for other
local filesystems. But I'm not sure if blkid works for char device and
ubifs (probably not).
>
> -g) group=$2 ; shift ;
> GROUP_LIST="$GROUP_LIST ${group//,/ }"
> diff --git a/common/config b/common/config
> index 59041a39..6c58e888 100644
> --- a/common/config
> +++ b/common/config
> @@ -336,6 +336,9 @@ _mount_opts()
> # We need to specify the size at mount, use 1G by default
> export MOUNT_OPTIONS="-o size=1G $TMPFS_MOUNT_OPTIONS"
> ;;
> + ubifs)
> + export MOUNT_OPTIONS=$UBIFS_MOUNT_OPTIONS
> + ;;
> *)
> ;;
> esac
> @@ -475,6 +478,10 @@ _check_device()
> if [ ! -d "$dev" ]; then
> _fatal "common/config: $name ($dev) is not a directory for overlay"
> fi
> + elif [ "$FSTYP" == "ubifs" ]; then
> + if [ ! -c "$dev" ]; then
> + _fatal "common/config: $name ($dev) is not a character device"
> + fi
> else
> _fatal "common/config: $name ($dev) is not a block device or a network filesystem"
This error message should be updated too. And turning this if-elif-fi
block to a case switch on $FSTYP seems cleaner.
And you need to setup MKFS_UBIFS_PROG and all other needed tools in
common/config too, and check if the tools are available in common/rc and
abort if the required tools are not met. e.g.
[ "$MKFS_EXT4_PROG" = "" ] && _fatal "mkfs.ext4 not found"
> fi
> diff --git a/common/encrypt b/common/encrypt
> index 723f1b11..b444c82d 100644
> --- a/common/encrypt
> +++ b/common/encrypt
> @@ -71,6 +71,9 @@ _scratch_mkfs_encrypted()
> ext4|f2fs)
> _scratch_mkfs -O encrypt
> ;;
> + ubifs)
> + ubiupdatevol ${SCRATCH_DEV} -t
> + ;;
Need a commont on this.
> *)
> _notrun "No encryption support for $FSTYP"
> ;;
> diff --git a/common/rc b/common/rc
> index 257b1903..6191a0c0 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -1556,6 +1556,15 @@ _require_scratch_nocheck()
> _notrun "this test requires a valid \$SCRATCH_MNT and unique $SCRATCH_DEV"
> fi
> ;;
> + ubifs)
> + # ubifs needs an UBI volume. This will be a char device, not a block device.
> + if [ ! -c "$SCRATCH_DEV" ]; then
> + _notrun "this test requires a valid UBI volume for \$SCRATCH_DEV"
> + fi
> + if [ ! -d "$SCRATCH_MNT" ]; then
> + _notrun "this test requires a valid \$SCRATCH_MNT"
> + fi
> + ;;
> *)
> if [ -z "$SCRATCH_DEV" -o "`_is_block_dev "$SCRATCH_DEV"`" = "" ]
> then
> @@ -1650,6 +1659,15 @@ _require_test()
> _notrun "this test requires a valid \$TEST_DIR and unique $TEST_DEV"
> fi
> ;;
> + ubifs)
> + # ubifs needs an UBI volume. This will be a char device, not a block device.
> + if [ ! -c "$TEST_DEV" ]; then
> + _notrun "this test requires a valid UBI volume for \$TEST_DEV"
> + fi
> + if [ ! -d "$TEST_DIR" ]; then
> + _notrun "this test requires a valid \$TEST_DIR"
> + fi
> + ;;
> *)
> if [ -z "$TEST_DEV" ] || [ "`_is_block_dev "$TEST_DEV"`" = "" ]
> then
> @@ -2490,6 +2508,9 @@ _check_test_fs()
> tmpfs)
> # no way to check consistency for tmpfs
> ;;
> + ubifs)
> + # there is no fsck program for ubifs yet
> + ;;
> *)
> _check_generic_filesystem $TEST_DEV
> ;;
> @@ -2539,6 +2560,9 @@ _check_scratch_fs()
> tmpfs)
> # no way to check consistency for tmpfs
> ;;
> + ubifs)
> + # there is no fsck program for ubifs yet
> + ;;
> *)
> _check_generic_filesystem $device
> ;;
> diff --git a/tests/generic/076 b/tests/generic/076
> index 64e69583..53e56a69 100755
> --- a/tests/generic/076
> +++ b/tests/generic/076
> @@ -56,7 +56,6 @@ _supported_fs generic
> _supported_os IRIX Linux
>
> _require_scratch
> -_require_block_device $SCRATCH_DEV
This test does "cat $SCRATCH_DEV >/dev/null &" which assumes
$SCRATCH_DEV is a local device, and test fails if it's a network device,
or an overlayfs pseudo mount device. And similarly generic/409 to
generic/411 fail if SCRATCH_DEV is a overlay pseudo mount device.
So I think what's needed here is a new helper that requires a local
device to make sure the given device resides on local host and is either
block or char device, e.g. something like
_require_local_device $SCRATCH_DEV
Thanks,
Eryu
>
> echo "*** init fs"
>
> diff --git a/tests/generic/409 b/tests/generic/409
> index 4bfedf73..3be58ba5 100755
> --- a/tests/generic/409
> +++ b/tests/generic/409
> @@ -64,7 +64,6 @@ _supported_fs generic
> _supported_os Linux
> _require_test
> _require_scratch
> -_require_block_device $SCRATCH_DEV
>
> fs_stress()
> {
> diff --git a/tests/generic/410 b/tests/generic/410
> index f2e0d1bb..e3a933ce 100755
> --- a/tests/generic/410
> +++ b/tests/generic/410
> @@ -72,7 +72,6 @@ _supported_fs generic
> _supported_os Linux
> _require_test
> _require_scratch
> -_require_block_device $SCRATCH_DEV
>
> fs_stress()
> {
> diff --git a/tests/generic/411 b/tests/generic/411
> index 7b2dd33b..08b861c5 100755
> --- a/tests/generic/411
> +++ b/tests/generic/411
> @@ -53,7 +53,6 @@ _supported_fs generic
> _supported_os Linux
> _require_test
> _require_scratch
> -_require_block_device $SCRATCH_DEV
>
> fs_stress()
> {
> --
> 2.12.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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 1/2] Add support for UBIFS
2017-05-17 11:53 ` Eryu Guan
@ 2017-05-17 18:45 ` Eric Biggers
2017-05-18 8:41 ` David Oberhollenzer
0 siblings, 1 reply; 10+ messages in thread
From: Eric Biggers @ 2017-05-17 18:45 UTC (permalink / raw)
To: Eryu Guan; +Cc: David Oberhollenzer, fstests, linux-mtd, richard
On Wed, May 17, 2017 at 07:53:55PM +0800, Eryu Guan wrote:
> On Wed, May 17, 2017 at 11:55:29AM +0200, David Oberhollenzer wrote:
> > This patch is mostly based on the previous attempts of Eric Biggers
> > and Dongsheng Yang at adding UBIFS support to xfstests.
> >
> > In addition to rebasing the previous attempts to recent xfstests,
> > most of the encryption tests now also support UBIFS with this patch.
> >
> > Since mkfs.ubifs doesn't support creating encryption-capable filesystems
> > yet, _scratch_mkfs_encrypted() is modified to wipe the underlying UBI
> > volume. The filesystem is then created when mounting the emtpy volume.
> >
> > Some tests that require SCRATCH_DEV to be a block device, despite
> > not using device-mapper or otherwise doing something block device
> > specific have that requirement removed.
> >
> > Signed-off-by: David Oberhollenzer <david.oberhollenzer@sigma-star.at>
>
> Thanks for the patch! I know only very little about ubifs (from previous
> reviews to attempts to add ubifs support to fstests), but it's still
> good to have some words to introduce ubifs briefly in commit log,
> especially the fact that it uses char device not block device.
>
> > ---
> > check | 2 ++
> > common/config | 7 +++++++
> > common/encrypt | 3 +++
> > common/rc | 24 ++++++++++++++++++++++++
> > tests/generic/076 | 1 -
> > tests/generic/409 | 1 -
> > tests/generic/410 | 1 -
> > tests/generic/411 | 1 -
> > 8 files changed, 36 insertions(+), 4 deletions(-)
> >
> > diff --git a/check b/check
> > index 9cef58b4..f8db3cd6 100755
> > --- a/check
> > +++ b/check
> > @@ -70,6 +70,7 @@ check options
> > -overlay test overlay
> > -pvfs2 test PVFS2
> > -tmpfs test TMPFS
> > + -ubifs test ubifs
> > -l line mode diff
> > -udiff show unified diff (default)
> > -n show me, do not run tests
> > @@ -267,6 +268,7 @@ while [ $# -gt 0 ]; do
> > -overlay) FSTYP=overlay; export OVERLAY=true ;;
> > -pvfs2) FSTYP=pvfs2 ;;
> > -tmpfs) FSTYP=tmpfs ;;
> > + -ubifs) FSTYP=ubifs ;;
>
> As being pointed out in previous reviews, it'll be great if we can probe
> ubifs from the char device if possible instead of adding new fs-specific
> option, just as what we're doing at the end of common/config for other
> local filesystems. But I'm not sure if blkid works for char device and
> ubifs (probably not).
>
It seems to work fine without the -ubifs option:
# blkid -o value -s TYPE /dev/ubi0_0
ubifs
# TEST_DEV=/dev/ubi0_0 TEST_DIR=/vdb ./check generic/001
FSTYP -- ubifs
PLATFORM -- Linux/x86_64 kvm-xfstests 4.12.0-rc1-xfstests-00083-ga844e08648f0-dirty
generic/001 [ 372.213194] run fstests generic/001 at 2017-05-17 11:43:47
8s
Ran: generic/001
Passed all 1 tests
- Eric
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] Add support for UBIFS
2017-05-17 18:45 ` Eric Biggers
@ 2017-05-18 8:41 ` David Oberhollenzer
2017-05-18 11:35 ` Eryu Guan
0 siblings, 1 reply; 10+ messages in thread
From: David Oberhollenzer @ 2017-05-18 8:41 UTC (permalink / raw)
To: Eric Biggers, Eryu Guan; +Cc: fstests, linux-mtd, richard
On 05/17/2017 01:53 PM, Eryu Guan wrote:
> On Wed, May 17, 2017 at 11:55:29AM +0200, David Oberhollenzer wrote:
>> -g) group=$2 ; shift ;
>> GROUP_LIST="$GROUP_LIST ${group//,/ }"
>> diff --git a/common/config b/common/config
>> index 59041a39..6c58e888 100644
>> --- a/common/config
>> +++ b/common/config
>> @@ -336,6 +336,9 @@ _mount_opts()
>> # We need to specify the size at mount, use 1G by default
>> export MOUNT_OPTIONS="-o size=1G $TMPFS_MOUNT_OPTIONS"
>> ;;
>> + ubifs)
>> + export MOUNT_OPTIONS=$UBIFS_MOUNT_OPTIONS
>> + ;;
>> *)
>> ;;
>> esac
>> @@ -475,6 +478,10 @@ _check_device()
>> if [ ! -d "$dev" ]; then
>> _fatal "common/config: $name ($dev) is not a directory for overlay"
>> fi
>> + elif [ "$FSTYP" == "ubifs" ]; then
>> + if [ ! -c "$dev" ]; then
>> + _fatal "common/config: $name ($dev) is not a character device"
>> + fi
>> else
>> _fatal "common/config: $name ($dev) is not a block device or a network filesystem"
>
> This error message should be updated too. And turning this if-elif-fi
> block to a case switch on $FSTYP seems cleaner.
>
> And you need to setup MKFS_UBIFS_PROG and all other needed tools in
> common/config too, and check if the tools are available in common/rc and
> abort if the required tools are not met. e.g.
>
> [ "$MKFS_EXT4_PROG" = "" ] && _fatal "mkfs.ext4 not found"
>
mkfs.ubifs itself isn't needed as empty ubi volumes are formated when
mounting them with UBIFS.
I think it would make sense to add a check for ubiupdatevol to
_require_scratch_encryption (used in _scratch_mkfs_encrypted to
whipe an existing volume).
On 05/17/2017 08:45 PM, Eric Biggers wrote:
> On Wed, May 17, 2017 at 07:53:55PM +0800, Eryu Guan wrote:
>> As being pointed out in previous reviews, it'll be great if we can probe
>> ubifs from the char device if possible instead of adding new fs-specific
>> option, just as what we're doing at the end of common/config for other
>> local filesystems. But I'm not sure if blkid works for char device and
>> ubifs (probably not).
>>
>
> It seems to work fine without the -ubifs option:
>
> # blkid -o value -s TYPE /dev/ubi0_0
> ubifs
I can't really reproduce this on my end. Neither on my Debian test VM,
nor on the OpenSUSE system that I'm working on right now. I get no
output from blkid, neither before nor after mounting the ubi volume.
To be fair, the Debian version (and thus its blkid version) is rather
old (blkid 1.0.0 (12-Feb-2003)), but the one on OpenSUSE seems to be
fairly recent:
$ blkid -v
blkid from util-linux 2.28 (libblkid 2.28., 12-Apr-2016)
Would it make sense to patch the check in common/config instead,
to default to ubifs if FSTYP is not set and the target is a
character device? Or simply require on FSTYP to be set in the
config file?
Thanks,
David
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 1/2] Add support for UBIFS
2017-05-18 8:41 ` David Oberhollenzer
@ 2017-05-18 11:35 ` Eryu Guan
0 siblings, 0 replies; 10+ messages in thread
From: Eryu Guan @ 2017-05-18 11:35 UTC (permalink / raw)
To: David Oberhollenzer; +Cc: Eric Biggers, fstests, linux-mtd, richard
On Thu, May 18, 2017 at 10:41:27AM +0200, David Oberhollenzer wrote:
> On 05/17/2017 01:53 PM, Eryu Guan wrote:
> > On Wed, May 17, 2017 at 11:55:29AM +0200, David Oberhollenzer wrote:
> >> -g) group=$2 ; shift ;
> >> GROUP_LIST="$GROUP_LIST ${group//,/ }"
> >> diff --git a/common/config b/common/config
> >> index 59041a39..6c58e888 100644
> >> --- a/common/config
> >> +++ b/common/config
> >> @@ -336,6 +336,9 @@ _mount_opts()
> >> # We need to specify the size at mount, use 1G by default
> >> export MOUNT_OPTIONS="-o size=1G $TMPFS_MOUNT_OPTIONS"
> >> ;;
> >> + ubifs)
> >> + export MOUNT_OPTIONS=$UBIFS_MOUNT_OPTIONS
> >> + ;;
> >> *)
> >> ;;
> >> esac
> >> @@ -475,6 +478,10 @@ _check_device()
> >> if [ ! -d "$dev" ]; then
> >> _fatal "common/config: $name ($dev) is not a directory for overlay"
> >> fi
> >> + elif [ "$FSTYP" == "ubifs" ]; then
> >> + if [ ! -c "$dev" ]; then
> >> + _fatal "common/config: $name ($dev) is not a character device"
> >> + fi
> >> else
> >> _fatal "common/config: $name ($dev) is not a block device or a network filesystem"
> >
> > This error message should be updated too. And turning this if-elif-fi
> > block to a case switch on $FSTYP seems cleaner.
> >
> > And you need to setup MKFS_UBIFS_PROG and all other needed tools in
> > common/config too, and check if the tools are available in common/rc and
> > abort if the required tools are not met. e.g.
> >
> > [ "$MKFS_EXT4_PROG" = "" ] && _fatal "mkfs.ext4 not found"
> >
>
> mkfs.ubifs itself isn't needed as empty ubi volumes are formated when
> mounting them with UBIFS.
But there's still a mkfs.ubifs binary, right? I searched fstests mail
archive and found that Dongsheng Yang did set MKFS_UBIFS_PROG in his
patch in 2015. Then I suspect if mkfs.ubifs binary is unavailable, then
_scratch_mkfs would complain mkfs.ubifs not found and fail the test.
If it's not needed or there's no mkfs.ubifs exists, then _scratch_mkfs
needs some updates I guess, and describe this in commit log too, because
it's too different from other block device based local filesystems.
>
> I think it would make sense to add a check for ubiupdatevol to
> _require_scratch_encryption (used in _scratch_mkfs_encrypted to
> whipe an existing volume).
Agreed.
>
>
> On 05/17/2017 08:45 PM, Eric Biggers wrote:
> > On Wed, May 17, 2017 at 07:53:55PM +0800, Eryu Guan wrote:
> >> As being pointed out in previous reviews, it'll be great if we can probe
> >> ubifs from the char device if possible instead of adding new fs-specific
> >> option, just as what we're doing at the end of common/config for other
> >> local filesystems. But I'm not sure if blkid works for char device and
> >> ubifs (probably not).
> >>
> >
> > It seems to work fine without the -ubifs option:
> >
> > # blkid -o value -s TYPE /dev/ubi0_0
> > ubifs
>
> I can't really reproduce this on my end. Neither on my Debian test VM,
> nor on the OpenSUSE system that I'm working on right now. I get no
> output from blkid, neither before nor after mounting the ubi volume.
>
> To be fair, the Debian version (and thus its blkid version) is rather
> old (blkid 1.0.0 (12-Feb-2003)), but the one on OpenSUSE seems to be
> fairly recent:
>
> $ blkid -v
> blkid from util-linux 2.28 (libblkid 2.28., 12-Apr-2016)
>
>
> Would it make sense to patch the check in common/config instead,
> to default to ubifs if FSTYP is not set and the target is a
> character device? Or simply require on FSTYP to be set in the
> config file?
If there's really no standard way to probe for ubifs, a "-ubifs" option
would be the only choice I think, and better to have some comments too.
Thanks,
Eryu
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 2/2] Accept failing with EPERM in addition to ENOKEY for rename without key
2017-05-17 9:55 Add UBIFS support to xfstests David Oberhollenzer
2017-05-17 9:55 ` [PATCH 1/2] Add support for UBIFS David Oberhollenzer
@ 2017-05-17 9:55 ` David Oberhollenzer
2017-05-17 19:21 ` Eric Biggers
2017-05-17 9:55 ` [PATCH] xfstests-bld: add experimental support for ubifs David Oberhollenzer
2017-05-17 19:05 ` Add UBIFS support to xfstests Eric Biggers
3 siblings, 1 reply; 10+ messages in thread
From: David Oberhollenzer @ 2017-05-17 9:55 UTC (permalink / raw)
To: fstests; +Cc: linux-mtd, richard, David Oberhollenzer
Some filesystems (e.g. UBIFS) fail with EPERM when trying to move a
file into an encrypted directory via cross rename, without having
access to the encryption key.
Since rename is perfectly allowed to return EPERM, which is also tested
for, and no precise specification seems to exist that clarifys when to
expect EPERM and when ENOKEY, this patch modifies the generic/398 test
to accept both, for the test case where the key is not available.
Signed-off-by: David Oberhollenzer <david.oberhollenzer@sigma-star.at>
---
tests/generic/398 | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/tests/generic/398 b/tests/generic/398
index b486b9c5..390c4380 100755
--- a/tests/generic/398
+++ b/tests/generic/398
@@ -43,6 +43,12 @@ _cleanup()
rm -f $tmp.*
}
+_filter_rename()
+{
+ # rename without key can also fail with EPERM instead of ENOKEY
+ sed -e "s/Operation not permitted/Required key not available/g"
+}
+
# get standard environment, filters and checks
. ./common/rc
. ./common/filter
@@ -149,9 +155,9 @@ efile1=$(find $edir1 -type f)
efile2=$(find $edir2 -type f)
echo -e "\n\n*** Exchange encrypted <=> encrypted without key ***"
-src/renameat2 -x $efile1 $efile2
+src/renameat2 -x $efile1 $efile2 |& _filter_rename
echo -e "\n*** Exchange encrypted <=> unencrypted without key ***"
-src/renameat2 -x $efile1 $udir/ufile
+src/renameat2 -x $efile1 $udir/ufile |& _filter_rename
# success, all done
status=0
--
2.12.0
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH 2/2] Accept failing with EPERM in addition to ENOKEY for rename without key
2017-05-17 9:55 ` [PATCH 2/2] Accept failing with EPERM in addition to ENOKEY for rename without key David Oberhollenzer
@ 2017-05-17 19:21 ` Eric Biggers
0 siblings, 0 replies; 10+ messages in thread
From: Eric Biggers @ 2017-05-17 19:21 UTC (permalink / raw)
To: David Oberhollenzer; +Cc: fstests, linux-mtd, richard
Hi David,
On Wed, May 17, 2017 at 11:55:30AM +0200, David Oberhollenzer wrote:
> Some filesystems (e.g. UBIFS) fail with EPERM when trying to move a
> file into an encrypted directory via cross rename, without having
> access to the encryption key.
>
> Since rename is perfectly allowed to return EPERM, which is also tested
> for, and no precise specification seems to exist that clarifys when to
> expect EPERM and when ENOKEY, this patch modifies the generic/398 test
> to accept both, for the test case where the key is not available.
>
Specifically, the operation is expected to fail for two logically independent
reasons:
- It shouldn't be possible to link or rename an unencrypted file into an
encrypted directory, as this violates the "one encryption policy per directory
tree" constraint (EPERM)
- It shouldn't be possible to link or rename files into an encrypted directory
without the directory's encryption key (ENOKEY)
Therefore it's not an ideal test, though it seemed worthwhile originally because
it was, in fact, broken in both ways, causing the operation to succeed :)
> Signed-off-by: David Oberhollenzer <david.oberhollenzer@sigma-star.at>
> ---
> tests/generic/398 | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/tests/generic/398 b/tests/generic/398
> index b486b9c5..390c4380 100755
> --- a/tests/generic/398
> +++ b/tests/generic/398
> @@ -43,6 +43,12 @@ _cleanup()
> rm -f $tmp.*
> }
>
> +_filter_rename()
> +{
> + # rename without key can also fail with EPERM instead of ENOKEY
> + sed -e "s/Operation not permitted/Required key not available/g"
> +}
> +
It doesn't matter much, but IMO it would make a little more sense to substitute
ENOKEY with EPERM instead of the other way around, then updating 398.out to
expect "Operation not permitted". The reason for this is that everything else
in generic/398 is testing for EPERM, because the test script is checking for
situations in which the "one encryption policy per directory tree" constraint
can be violated, and such violations normally result in EPERM.
Eric
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] xfstests-bld: add experimental support for ubifs
2017-05-17 9:55 Add UBIFS support to xfstests David Oberhollenzer
2017-05-17 9:55 ` [PATCH 1/2] Add support for UBIFS David Oberhollenzer
2017-05-17 9:55 ` [PATCH 2/2] Accept failing with EPERM in addition to ENOKEY for rename without key David Oberhollenzer
@ 2017-05-17 9:55 ` David Oberhollenzer
2017-05-17 19:05 ` Add UBIFS support to xfstests Eric Biggers
3 siblings, 0 replies; 10+ messages in thread
From: David Oberhollenzer @ 2017-05-17 9:55 UTC (permalink / raw)
To: fstests; +Cc: linux-mtd, richard, Eric Biggers, David Oberhollenzer
From: Eric Biggers <ebiggers@google.com>
This experimental patch adds ubifs support to kvm-xfstests and
gce-xfstests. Unlike most filesystems ubifs is not block-device based.
Instead it uses an UBI volume, which is layered above an MTD (raw flash)
device. Fortunately it's possible to use the block2mtd kernel module to
emulate an MTD device given a block device. This patch adds an ubifs
xfstests-bld config which uses block2mtd to create MTD devices, then UBI
devices and UBI volumes, that are backed by the kvm-xfstests or
gce-xfstests block devices.
Here are the steps to run kvm-xfstests on ubifs after this patch:
1. Apply ubifs support patch to xfstests
2. Build a kernel based on the existing xfstests-bld config but also with:
CONFIG_MTD=y
CONFIG_MTD_BLOCK2MTD=y
CONFIG_MTD_UBI=y
CONFIG_UBIFS_FS=y
CONFIG_UBIFS_FS_ENCRYPTION=y
3. Run: kvm-xfstests -c ubifs -g <group> ...
I also tried using QEMU to emulate a MTD device directly, but I couldn't
get it to work; and that definitely won't work for gce-xfstests.
Known issues (other than real test failures):
- The first time ubifs tests are run, it takes a long time to format the
emulated MTD devices. I don't know of a way around this other than
simply using smaller devices.
- Using the same block devices for other filesystems and for MTD
emulation can cause problems, e.g. without rebooting the VM, it's not
possible to run tests on another filesystem after ubifs because
block2mtd does not support removing MTD devices.
Signed-off-by: Eric Biggers <ebiggers@google.com>
Signed-off-by: David Oberhollenzer <david.oberhollenzer@sigma-star.at>
---
.../files/root/fs/ubifs/cfg/all.list | 1 +
.../test-appliance/files/root/fs/ubifs/cfg/default | 11 ++
.../test-appliance/files/root/fs/ubifs/config | 167 +++++++++++++++++++++
kvm-xfstests/test-appliance/files/root/runtests.sh | 4 +-
kvm-xfstests/test-appliance/xfstests-packages | 1 +
5 files changed, 182 insertions(+), 2 deletions(-)
create mode 100644 kvm-xfstests/test-appliance/files/root/fs/ubifs/cfg/all.list
create mode 100644 kvm-xfstests/test-appliance/files/root/fs/ubifs/cfg/default
create mode 100644 kvm-xfstests/test-appliance/files/root/fs/ubifs/config
diff --git a/kvm-xfstests/test-appliance/files/root/fs/ubifs/cfg/all.list b/kvm-xfstests/test-appliance/files/root/fs/ubifs/cfg/all.list
new file mode 100644
index 0000000..4ad96d5
--- /dev/null
+++ b/kvm-xfstests/test-appliance/files/root/fs/ubifs/cfg/all.list
@@ -0,0 +1 @@
+default
diff --git a/kvm-xfstests/test-appliance/files/root/fs/ubifs/cfg/default b/kvm-xfstests/test-appliance/files/root/fs/ubifs/cfg/default
new file mode 100644
index 0000000..be3ba67
--- /dev/null
+++ b/kvm-xfstests/test-appliance/files/root/fs/ubifs/cfg/default
@@ -0,0 +1,11 @@
+SIZE=small
+
+# set up UBI volumes for our block devices
+export TEST_DEV=$(__blkdev_to_ubi_volume $SM_TST_DEV)
+export TEST_DIR=$SM_TST_MNT
+export SCRATCH_DEV=$(__blkdev_to_ubi_volume $SM_SCR_DEV)
+export SCRATCH_MNT=$SM_SCR_MNT
+
+export MKFS_OPTIONS=""
+export UBIFS_MOUNT_OPTIONS=""
+TESTNAME="ubifs"
diff --git a/kvm-xfstests/test-appliance/files/root/fs/ubifs/config b/kvm-xfstests/test-appliance/files/root/fs/ubifs/config
new file mode 100644
index 0000000..91d8de6
--- /dev/null
+++ b/kvm-xfstests/test-appliance/files/root/fs/ubifs/config
@@ -0,0 +1,167 @@
+#
+# Configuration file for ubifs
+#
+
+DEFAULT_MKFS_OPTIONS=""
+
+function check_filesystem()
+{
+ # there is no fsck.ubifs yet
+ :
+}
+
+# Find the MTD device which is backed by the specified block device
+function __mtd_find()
+{
+ local blkdev=$1 mtd_dir
+
+ for mtd_dir in /sys/class/mtd/*; do
+ if [[ $mtd_dir =~ ^.*/mtd[0-9]+$ ]] &&
+ [[ $(awk '/^block2mtd:/{print $2}' $mtd_dir/name) == $blkdev ]]
+ then
+ echo /dev/$(basename $mtd_dir)
+ return
+ fi
+ done
+}
+
+# Find or create the MTD device which is backed by the specified block device
+function __mtd_find_or_create()
+{
+ local blkdev=$1 mtd
+
+ if [ ! -e /sys/module/block2mtd ]; then
+ echo 1>&2 "Error: CONFIG_MTD_BLOCK2MTD=y is required to emulate flash device for ubifs!"
+ return
+ fi
+
+ mtd=$(__mtd_find $blkdev)
+ if [ ! -c "$mtd" ]; then
+ # Create a new block2mtd device. For now choose an eraseblock size of
+ # 128 KiB. I'm not sure if that's the best value or not.
+ echo "$blkdev,131072" > /sys/module/block2mtd/parameters/block2mtd
+ mtd=$(__mtd_find $blkdev)
+ fi
+ echo $mtd
+}
+
+# Find the UBI device which has the specified mtd device attached
+function __ubi_find()
+{
+ local mtd_num="${1#/dev/mtd}" ubi_dir
+
+ for ubi_dir in /sys/class/ubi/*; do
+ if [[ $ubi_dir =~ ^.*/ubi[0-9]+$ ]] &&
+ [[ $(<$ubi_dir/mtd_num) == $mtd_num ]]
+ then
+ echo /dev/$(basename $ubi_dir)
+ return
+ fi
+ done
+}
+
+# Find or create the UBI device which has the specified mtd device attached
+function __ubi_find_or_create()
+{
+ local mtd="$1" ubi
+
+ ubi=$(__ubi_find $mtd)
+ if [ ! -c "$ubi" ]; then
+ if ! ubiattach -p $mtd &> /dev/null; then
+ # ubiattach didn't work; try formatting the MTD device first.
+ # Note: since this requires writing to the entire device, it may be
+ # *very* slow...
+ echo 1>&2 "Formatting $mtd with ubiformat (this may take a while!)..."
+ ubiformat -e 0 -y $mtd > /dev/null
+ ubiattach -p $mtd > /dev/null
+ fi
+ ubi=$(__ubi_find $mtd)
+ fi
+ echo $ubi
+}
+
+
+#
+# Find or create the UBI volume backed by the specified block device.
+#
+# There are four types of devices in play here. Here's an example:
+#
+# /dev/vdb --- Block device
+# /dev/mtd0 --- MTD device backed by /dev/vdb using block2mtd
+# /dev/ubi0 --- UBI device to which /dev/mtd0 is attached
+# /dev/ubi0_0 --- UBI volume within /dev/ubi0
+#
+# In this example, this function would take in /dev/vdb as $1 and echo back
+# /dev/ubi0_0, creating it and the two intermediary devices if needed.
+#
+function __blkdev_to_ubi_volume()
+{
+ local blkdev=$1 mtd ubi ubivol
+
+ mtd=$(__mtd_find_or_create $blkdev)
+ if [ ! -c "$mtd" ]; then
+ echo 1>&2 "Error: Failed to create MTD device from $blkdev!"
+ return
+ fi
+
+ ubi=$(__ubi_find_or_create $mtd)
+ if [ ! -c "$ubi" ]; then
+ echo 1>&2 "Error: Failed create UBI device from $mtd!"
+ return
+ fi
+
+ ubivol=${ubi}_0
+ if [ ! -c "$ubivol" ]; then
+ ubimkvol $ubi -N vol -m > /dev/null
+ if [ ! -c "$ubivol" ]; then
+ echo 1>&2 "Error: Failed to create UBI volume $ubivol from $ubi"
+ return
+ fi
+ fi
+ echo $ubivol
+}
+
+function format_filesystem()
+{
+ local dev="$1"
+ local opts="$2"
+
+ mkfs.ubifs -y $opts "$dev"
+}
+
+function setup_mount_opts()
+{
+ if test -n "$MNTOPTS" ; then
+ if test -n "$UBIFS_MOUNT_OPTIONS" ; then
+ UBIFS_MOUNT_OPTIONS="$UBIFS_MOUNT_OPTIONS,$MNTOPTS"
+ else
+ UBIFS_MOUNT_OPTIONS="-o $MNTOPTS"
+ fi
+ fi
+}
+
+function get_mkfs_opts()
+{
+ echo "$MKFS_OPTIONS"
+}
+
+function show_mkfs_opts()
+{
+ echo MKFS_OPTIONS: "$MKFS_OPTIONS"
+}
+
+function show_mount_opts()
+{
+ echo UBIFS_MOUNT_OPTIONS: "$UBIFS_MOUNT_OPTIONS"
+}
+
+function test_name_alias()
+{
+ echo "$1"
+}
+
+function reset_vars()
+{
+ unset UBIFS_MOUNT_OPTIONS
+ unset MKFS_OPTIONS
+}
diff --git a/kvm-xfstests/test-appliance/files/root/runtests.sh b/kvm-xfstests/test-appliance/files/root/runtests.sh
index 8363427..9577f3e 100755
--- a/kvm-xfstests/test-appliance/files/root/runtests.sh
+++ b/kvm-xfstests/test-appliance/files/root/runtests.sh
@@ -272,11 +272,11 @@ do
*/ovl) ;;
*:/*) ;;
*)
- if ! test -b $TEST_DEV ; then
+ if ! [ -b $TEST_DEV -o -c $TEST_DEV ]; then
echo "Test device $TEST_DEV does not exist, skipping $i config"
continue
fi
- if ! test -b $SCRATCH_DEV ; then
+ if ! [ -b $SCRATCH_DEV -o -c $SCRATCH_DEV ]; then
echo "Scratch device $SCRATCH_DEV does not exist, skipping $i config"
continue
fi
diff --git a/kvm-xfstests/test-appliance/xfstests-packages b/kvm-xfstests/test-appliance/xfstests-packages
index afb6c5d..2f8e4b0 100644
--- a/kvm-xfstests/test-appliance/xfstests-packages
+++ b/kvm-xfstests/test-appliance/xfstests-packages
@@ -30,3 +30,4 @@ time
uuid-runtime
udev
xz-utils
+mtd-utils
--
2.12.0
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: Add UBIFS support to xfstests
2017-05-17 9:55 Add UBIFS support to xfstests David Oberhollenzer
` (2 preceding siblings ...)
2017-05-17 9:55 ` [PATCH] xfstests-bld: add experimental support for ubifs David Oberhollenzer
@ 2017-05-17 19:05 ` Eric Biggers
3 siblings, 0 replies; 10+ messages in thread
From: Eric Biggers @ 2017-05-17 19:05 UTC (permalink / raw)
To: David Oberhollenzer; +Cc: fstests, linux-mtd, richard
Hi David,
On Wed, May 17, 2017 at 11:55:28AM +0200, David Oberhollenzer wrote:
>
> Modifying the xfstests-bld patch to use nandsim (which simulates a NAND
> flash MTD device) instead of block2mtd has been considered, however the
> default size of 128MiB is slightly too small for generic/129.
>
> Nandsim does not have an option to directly specify the size of the
> backing buffer but instead requires a set of NAND ID bytes that imply
> (among other things) the size of the chip (example, see 1). The ID bytes
> would have to be added to the kernel arguments for the UBIFS case. It
> might be desirable to document this somehow, for the next person who
> has to adjust the size, once a new test exhausts it. Also, it would be
> ultimately limted by available RAM and what can be encoded using the
> NAND ID bytes.
>
> So right now, it looks like block2mtd is the simplest option for getting
> xfstests-bld to work with UBIFS.
>
It probably would be a good idea to document this decision somewhere more
permanent, maybe in a comment in
kvm-xfstests/test-appliance/files/root/fs/ubifs/config, which is where the
block2mtd devices get set up.
Or, it could be useful to add a documentation file for UBIFS in the xfstests-bld
documentation directory, which would also document any "gotchas" specific to how
UBIFS support is implemented in the test appliances, e.g the two issues I listed
in my patch description. Also the fact that you need to configure your kernel
with UBIFS and block2mtd support. Ideally we'd just fix these issues, though,
so there would be nothing special to document. (We could at least solve the
kernel configuration issue by renaming the example configs in the
kernel-configs/ directory from "ext4-" to "xfstests-", then enabling support for
ubifs, f2fs, btrfs, etc. in them.)
Eric
^ permalink raw reply [flat|nested] 10+ messages in thread