[PATCH] common/encrypt: Create an encrypted equivalent of _scratch_mkfs_sized

[PATCH] common/encrypt: Create an encrypted equivalent of _scratch_mkfs_sized

[Ari Sundholm]

Test case generic/399 hardcodes "-O encrypt" in MKFS_OPTIONS when
calling _scratch_mkfs_sized, which only works with the mkfs of certain
filesystems. Create a new helper, _scratch_mkfs_sized_encrypted, for
handling the differences between the mkfs tools of different
filesystems. It also allows those filesystems whose mkfs doesn't accept
"-O encrypt" to skip the test gracefully until proper support is added
for them in the helper.

Signed-off-by: Ari Sundholm <ari@tuxera.com>
---
 common/encrypt    | 12 ++++++++++++
 tests/generic/399 |  3 +--
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/common/encrypt b/common/encrypt
index a6fd89d..189c59e 100644
--- a/common/encrypt
+++ b/common/encrypt
@@ -81,6 +81,18 @@ _scratch_mkfs_encrypted()
 	esac
 }
 
+_scratch_mkfs_sized_encrypted()
+{
+	case $FSTYP in
+	ext4|f2fs)
+		MKFS_OPTIONS="$MKFS_OPTIONS -O encrypt" _scratch_mkfs_sized $*
+		;;
+	*)
+		_notrun "Filesystem $FSTYP not supported in _scratch_mkfs_sized_encrypted"
+		;;
+	esac
+}
+
 # Give the invoking shell a new session keyring.  This makes any keys we add to
 # the session keyring scoped to the lifetime of the test script.
 _new_session_keyring()
diff --git a/tests/generic/399 b/tests/generic/399
index 8f5fcdc..70896e4 100755
--- a/tests/generic/399
+++ b/tests/generic/399
@@ -76,8 +76,7 @@ fs_size_in_mb=64
 fs_size=$((fs_size_in_mb * 1024 * 1024))
 dd if=/dev/zero of=$SCRATCH_DEV bs=$((1024 * 1024)) \
 	count=$fs_size_in_mb &>> $seqres.full
-MKFS_OPTIONS="$MKFS_OPTIONS -O encrypt" \
-	_scratch_mkfs_sized $fs_size &>> $seqres.full
+_scratch_mkfs_sized_encrypted $fs_size &>> $seqres.full
 _scratch_mount
 
 keydesc=$(_generate_encryption_key)
-- 
2.7.4

Re: [PATCH] common/encrypt: Create an encrypted equivalent of _scratch_mkfs_sized

[Dave Chinner]

On Wed, Dec 20, 2017 at 07:46:40PM +0200, Ari Sundholm wrote:
> Test case generic/399 hardcodes "-O encrypt" in MKFS_OPTIONS when
> calling _scratch_mkfs_sized, which only works with the mkfs of certain
> filesystems. Create a new helper, _scratch_mkfs_sized_encrypted, for
> handling the differences between the mkfs tools of different
> filesystems. It also allows those filesystems whose mkfs doesn't accept
> "-O encrypt" to skip the test gracefully until proper support is added
> for them in the helper.
> 
> Signed-off-by: Ari Sundholm <ari@tuxera.com>
> ---
>  common/encrypt    | 12 ++++++++++++
>  tests/generic/399 |  3 +--
>  2 files changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/common/encrypt b/common/encrypt
> index a6fd89d..189c59e 100644
> --- a/common/encrypt
> +++ b/common/encrypt
> @@ -81,6 +81,18 @@ _scratch_mkfs_encrypted()
>  	esac
>  }
>  
> +_scratch_mkfs_sized_encrypted()
> +{
> +	case $FSTYP in
> +	ext4|f2fs)
> +		MKFS_OPTIONS="$MKFS_OPTIONS -O encrypt" _scratch_mkfs_sized $*
> +		;;

This does not need to screw around with MKFS_OPTIONS. This:

		_scratch_mkfs_sized -O encrypt $*

Will do just fine.

Also, _scratch_mkfs_encrypted() supports UBIFS, and this new
function doesn't. Seems like it should to me...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] common/encrypt: Create an encrypted equivalent of _scratch_mkfs_sized

[Ari Sundholm]

Hi!

Thank you for your comments. Please see below.

On 12/21/2017 03:38 AM, Dave Chinner wrote:
> On Wed, Dec 20, 2017 at 07:46:40PM +0200, Ari Sundholm wrote:
>> Test case generic/399 hardcodes "-O encrypt" in MKFS_OPTIONS when
>> calling _scratch_mkfs_sized, which only works with the mkfs of certain
>> filesystems. Create a new helper, _scratch_mkfs_sized_encrypted, for
>> handling the differences between the mkfs tools of different
>> filesystems. It also allows those filesystems whose mkfs doesn't accept
>> "-O encrypt" to skip the test gracefully until proper support is added
>> for them in the helper.
>>
>> Signed-off-by: Ari Sundholm <ari@tuxera.com>
>> ---
>>   common/encrypt    | 12 ++++++++++++
>>   tests/generic/399 |  3 +--
>>   2 files changed, 13 insertions(+), 2 deletions(-)
>>
>> diff --git a/common/encrypt b/common/encrypt
>> index a6fd89d..189c59e 100644
>> --- a/common/encrypt
>> +++ b/common/encrypt
>> @@ -81,6 +81,18 @@ _scratch_mkfs_encrypted()
>>   	esac
>>   }
>>   
>> +_scratch_mkfs_sized_encrypted()
>> +{
>> +	case $FSTYP in
>> +	ext4|f2fs)
>> +		MKFS_OPTIONS="$MKFS_OPTIONS -O encrypt" _scratch_mkfs_sized $*
>> +		;;
> 
> This does not need to screw around with MKFS_OPTIONS. This:
> 
> 		_scratch_mkfs_sized -O encrypt $*
> 
> Will do just fine.

Hmm, I don't see how that could work. At the moment, _scratch_mkfs_sized 
only takes and uses two arguments, one of which is optional. AFAICS, all 
additional mkfs options need to be passed using MKFS_OPTIONS to 
_scratch_mkfs_sized.

As for using $* instead of $1 $2, I am hoping that will avoid having to 
change this new function even if the usage of _scratch_mkfs_sized changes.

> Also, _scratch_mkfs_encrypted() supports UBIFS, and this new
> function doesn't. Seems like it should to me...

_scratch_mkfs_sized does not support ubifs, though, so the operation 
would _notrun anyway. I'm not familiar with ubifs or its tools, but I'll 
see if I can find out how to implement support for ubifs in 
_scratch_mkfs_sized.

> Cheers,
> 
> Dave.
> 

Thanks,
Ari Sundholm
ari@tuxera.com

Re: [PATCH] common/encrypt: Create an encrypted equivalent of _scratch_mkfs_sized

[Ari Sundholm]

On 12/21/2017 03:52 PM, Ari Sundholm wrote:
> Hi!
> 
> Thank you for your comments. Please see below.
> 
> On 12/21/2017 03:38 AM, Dave Chinner wrote:
>> On Wed, Dec 20, 2017 at 07:46:40PM +0200, Ari Sundholm wrote:
>>> Test case generic/399 hardcodes "-O encrypt" in MKFS_OPTIONS when
>>> calling _scratch_mkfs_sized, which only works with the mkfs of certain
>>> filesystems. Create a new helper, _scratch_mkfs_sized_encrypted, for
>>> handling the differences between the mkfs tools of different
>>> filesystems. It also allows those filesystems whose mkfs doesn't accept
>>> "-O encrypt" to skip the test gracefully until proper support is added
>>> for them in the helper.
>>>
>>> Signed-off-by: Ari Sundholm <ari@tuxera.com>
>>> ---
>>>   common/encrypt    | 12 ++++++++++++
>>>   tests/generic/399 |  3 +--
>>>   2 files changed, 13 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/common/encrypt b/common/encrypt
>>> index a6fd89d..189c59e 100644
>>> --- a/common/encrypt
>>> +++ b/common/encrypt
>>> @@ -81,6 +81,18 @@ _scratch_mkfs_encrypted()
>>>       esac
>>>   }
>>> +_scratch_mkfs_sized_encrypted()
>>> +{
>>> +    case $FSTYP in
>>> +    ext4|f2fs)
>>> +        MKFS_OPTIONS="$MKFS_OPTIONS -O encrypt" _scratch_mkfs_sized $*
>>> +        ;;
>>
>> This does not need to screw around with MKFS_OPTIONS. This:
>>
>>         _scratch_mkfs_sized -O encrypt $*
>>
>> Will do just fine.
> 
> Hmm, I don't see how that could work. At the moment, _scratch_mkfs_sized 
> only takes and uses two arguments, one of which is optional. AFAICS, all 
> additional mkfs options need to be passed using MKFS_OPTIONS to 
> _scratch_mkfs_sized.
> 
> As for using $* instead of $1 $2, I am hoping that will avoid having to 
> change this new function even if the usage of _scratch_mkfs_sized changes.
> 
>> Also, _scratch_mkfs_encrypted() supports UBIFS, and this new
>> function doesn't. Seems like it should to me...
> 
> _scratch_mkfs_sized does not support ubifs, though, so the operation 
> would _notrun anyway. I'm not familiar with ubifs or its tools, but I'll 
> see if I can find out how to implement support for ubifs in 
> _scratch_mkfs_sized.
> 

I looked into implementing ubifs support for _scratch_mkfs_sized and 
_scratch_mkfs_sized_encrypted. It doesn't seem there is a very good way 
of doing this, apart from removing the existing volume and recreating 
it, something like this:
	ubirmvol $SCRATCH_DEV -n 0
	ubimkvol $SCRATCH_DEV -n 0 -N foobar -s SIZE

This has the problem that we may not really know what the volume ID 
should be. Can we trust it to be 0? The other issue is that $SCRATCH_DEV 
probably points to a UBI volume instead of a UBI device. I guess ubinfo 
(or just sed'ing out the volume suffix) could be used to get the 
underlying UBI device.

I think supporting this should be in a separate patch.

Best regards,
Ari Sundholm
ari@tuxera.com

>> Cheers,
>>
>> Dave.
>>
> 
> Thanks,
> Ari Sundholm
> ari@tuxera.com
> -- 
> 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] common/encrypt: Create an encrypted equivalent of _scratch_mkfs_sized

[Dave Chinner]

On Thu, Dec 21, 2017 at 03:52:00PM +0200, Ari Sundholm wrote:
> Hi!
> 
> Thank you for your comments. Please see below.
> 
> On 12/21/2017 03:38 AM, Dave Chinner wrote:
> >On Wed, Dec 20, 2017 at 07:46:40PM +0200, Ari Sundholm wrote:
> >>Test case generic/399 hardcodes "-O encrypt" in MKFS_OPTIONS when
> >>calling _scratch_mkfs_sized, which only works with the mkfs of certain
> >>filesystems. Create a new helper, _scratch_mkfs_sized_encrypted, for
> >>handling the differences between the mkfs tools of different
> >>filesystems. It also allows those filesystems whose mkfs doesn't accept
> >>"-O encrypt" to skip the test gracefully until proper support is added
> >>for them in the helper.
> >>
> >>Signed-off-by: Ari Sundholm <ari@tuxera.com>
> >>---
> >>  common/encrypt    | 12 ++++++++++++
> >>  tests/generic/399 |  3 +--
> >>  2 files changed, 13 insertions(+), 2 deletions(-)
> >>
> >>diff --git a/common/encrypt b/common/encrypt
> >>index a6fd89d..189c59e 100644
> >>--- a/common/encrypt
> >>+++ b/common/encrypt
> >>@@ -81,6 +81,18 @@ _scratch_mkfs_encrypted()
> >>  	esac
> >>  }
> >>+_scratch_mkfs_sized_encrypted()
> >>+{
> >>+	case $FSTYP in
> >>+	ext4|f2fs)
> >>+		MKFS_OPTIONS="$MKFS_OPTIONS -O encrypt" _scratch_mkfs_sized $*
> >>+		;;
> >
> >This does not need to screw around with MKFS_OPTIONS. This:
> >
> >		_scratch_mkfs_sized -O encrypt $*
> >
> >Will do just fine.
> 
> Hmm, I don't see how that could work. At the moment,
> _scratch_mkfs_sized only takes and uses two arguments, one of which
> is optional. AFAICS, all additional mkfs options need to be passed
> using MKFS_OPTIONS to _scratch_mkfs_sized.

Oh, I was under the impression that got fixed some time ago.
Screwing with MKFS_OPTIONS means defeats some of the test specific
mkfs option conflict resolution that some filesystem have.

i.e. when the options specified by the test cause problems with test
run specified MKFS_OPTIONS, the MKFS_OPTIONS get dropped and just
the test specific options are used. Setting random test options in
MKFS_OPTIONS can cause _scratch_mkfs_sized to not use the options
specified by the test at all...

> As for using $* instead of $1 $2, I am hoping that will avoid having
> to change this new function even if the usage of _scratch_mkfs_sized
> changes.
> 
> >Also, _scratch_mkfs_encrypted() supports UBIFS, and this new
> >function doesn't. Seems like it should to me...
> 
> _scratch_mkfs_sized does not support ubifs, though, so the operation
> would _notrun anyway. I'm not familiar with ubifs or its tools, but
> I'll see if I can find out how to implement support for ubifs in
> _scratch_mkfs_sized.

Ok, that should be mentioned in the commit message so people know
why it isn't supported despite supporting encryption...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

[PATCH v2] common/encrypt: Create an encrypted equivalent of _scratch_mkfs_sized

[Ari Sundholm]

Test case generic/399 hardcodes "-O encrypt" in MKFS_OPTIONS when
calling _scratch_mkfs_sized, which only works with the mkfs of certain
filesystems. Create a new helper, _scratch_mkfs_sized_encrypted, for
handling the differences between the mkfs tools of different
filesystems. It also allows those filesystems whose mkfs doesn't accept
"-O encrypt" to skip the test gracefully until proper support is added
for them in the helper.

ubifs is not supported in the new helper despite supporting encryption,
as _scratch_mkfs_sized has no ubifs support and adding that should be
done in a separate patch.

Signed-off-by: Ari Sundholm <ari@tuxera.com>
---
 common/encrypt    | 12 ++++++++++++
 tests/generic/399 |  3 +--
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/common/encrypt b/common/encrypt
index a6fd89d..189c59e 100644
--- a/common/encrypt
+++ b/common/encrypt
@@ -81,6 +81,18 @@ _scratch_mkfs_encrypted()
 	esac
 }
 
+_scratch_mkfs_sized_encrypted()
+{
+	case $FSTYP in
+	ext4|f2fs)
+		MKFS_OPTIONS="$MKFS_OPTIONS -O encrypt" _scratch_mkfs_sized $*
+		;;
+	*)
+		_notrun "Filesystem $FSTYP not supported in _scratch_mkfs_sized_encrypted"
+		;;
+	esac
+}
+
 # Give the invoking shell a new session keyring.  This makes any keys we add to
 # the session keyring scoped to the lifetime of the test script.
 _new_session_keyring()
diff --git a/tests/generic/399 b/tests/generic/399
index 8f5fcdc..70896e4 100755
--- a/tests/generic/399
+++ b/tests/generic/399
@@ -76,8 +76,7 @@ fs_size_in_mb=64
 fs_size=$((fs_size_in_mb * 1024 * 1024))
 dd if=/dev/zero of=$SCRATCH_DEV bs=$((1024 * 1024)) \
 	count=$fs_size_in_mb &>> $seqres.full
-MKFS_OPTIONS="$MKFS_OPTIONS -O encrypt" \
-	_scratch_mkfs_sized $fs_size &>> $seqres.full
+_scratch_mkfs_sized_encrypted $fs_size &>> $seqres.full
 _scratch_mount
 
 keydesc=$(_generate_encryption_key)
-- 
2.7.4

Re: [PATCH] common/encrypt: Create an encrypted equivalent of _scratch_mkfs_sized

[Ari Sundholm]

On 12/21/2017 11:46 PM, Dave Chinner wrote:
> On Thu, Dec 21, 2017 at 03:52:00PM +0200, Ari Sundholm wrote:
>> Hi!
>>
>> Thank you for your comments. Please see below.
>>
>> On 12/21/2017 03:38 AM, Dave Chinner wrote:
>>> On Wed, Dec 20, 2017 at 07:46:40PM +0200, Ari Sundholm wrote:
>>>> Test case generic/399 hardcodes "-O encrypt" in MKFS_OPTIONS when
>>>> calling _scratch_mkfs_sized, which only works with the mkfs of certain
>>>> filesystems. Create a new helper, _scratch_mkfs_sized_encrypted, for
>>>> handling the differences between the mkfs tools of different
>>>> filesystems. It also allows those filesystems whose mkfs doesn't accept
>>>> "-O encrypt" to skip the test gracefully until proper support is added
>>>> for them in the helper.
>>>>
>>>> Signed-off-by: Ari Sundholm <ari@tuxera.com>
>>>> ---
>>>>   common/encrypt    | 12 ++++++++++++
>>>>   tests/generic/399 |  3 +--
>>>>   2 files changed, 13 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/common/encrypt b/common/encrypt
>>>> index a6fd89d..189c59e 100644
>>>> --- a/common/encrypt
>>>> +++ b/common/encrypt
>>>> @@ -81,6 +81,18 @@ _scratch_mkfs_encrypted()
>>>>   	esac
>>>>   }
>>>> +_scratch_mkfs_sized_encrypted()
>>>> +{
>>>> +	case $FSTYP in
>>>> +	ext4|f2fs)
>>>> +		MKFS_OPTIONS="$MKFS_OPTIONS -O encrypt" _scratch_mkfs_sized $*
>>>> +		;;
>>>
>>> This does not need to screw around with MKFS_OPTIONS. This:
>>>
>>> 		_scratch_mkfs_sized -O encrypt $*
>>>
>>> Will do just fine.
>>
>> Hmm, I don't see how that could work. At the moment,
>> _scratch_mkfs_sized only takes and uses two arguments, one of which
>> is optional. AFAICS, all additional mkfs options need to be passed
>> using MKFS_OPTIONS to _scratch_mkfs_sized.
> 
> Oh, I was under the impression that got fixed some time ago.
> Screwing with MKFS_OPTIONS means defeats some of the test specific
> mkfs option conflict resolution that some filesystem have.
> 
> i.e. when the options specified by the test cause problems with test
> run specified MKFS_OPTIONS, the MKFS_OPTIONS get dropped and just
> the test specific options are used. Setting random test options in
> MKFS_OPTIONS can cause _scratch_mkfs_sized to not use the options
> specified by the test at all...

Yes, we were actually bitten by this a while ago. It is a bit tricky to 
solve this for _scratch_mkfs_sized given that it has an optional 
argument that we do not want to confuse with the extra arguments the 
test case specifies. For this reason it can't work like _scratch_mkfs does.

>> As for using $* instead of $1 $2, I am hoping that will avoid having
>> to change this new function even if the usage of _scratch_mkfs_sized
>> changes.
>>
>>> Also, _scratch_mkfs_encrypted() supports UBIFS, and this new
>>> function doesn't. Seems like it should to me...
>>
>> _scratch_mkfs_sized does not support ubifs, though, so the operation
>> would _notrun anyway. I'm not familiar with ubifs or its tools, but
>> I'll see if I can find out how to implement support for ubifs in
>> _scratch_mkfs_sized.
> 
> Ok, that should be mentioned in the commit message so people know
> why it isn't supported despite supporting encryption...

Fair enough. Sent v2 with an amended commit message.

> Cheers,
> 
> Dave.
> 

Thanks,
Ari Sundholm
ari@tuxera.com

Re: [PATCH] common/encrypt: Create an encrypted equivalent of _scratch_mkfs_sized

[Eryu Guan]

On Fri, Dec 22, 2017 at 08:46:57AM +1100, Dave Chinner wrote:
> On Thu, Dec 21, 2017 at 03:52:00PM +0200, Ari Sundholm wrote:
> > Hi!
> > 
> > Thank you for your comments. Please see below.
> > 
> > On 12/21/2017 03:38 AM, Dave Chinner wrote:
> > >On Wed, Dec 20, 2017 at 07:46:40PM +0200, Ari Sundholm wrote:
> > >>Test case generic/399 hardcodes "-O encrypt" in MKFS_OPTIONS when
> > >>calling _scratch_mkfs_sized, which only works with the mkfs of certain
> > >>filesystems. Create a new helper, _scratch_mkfs_sized_encrypted, for
> > >>handling the differences between the mkfs tools of different
> > >>filesystems. It also allows those filesystems whose mkfs doesn't accept
> > >>"-O encrypt" to skip the test gracefully until proper support is added
> > >>for them in the helper.
> > >>
> > >>Signed-off-by: Ari Sundholm <ari@tuxera.com>
> > >>---
> > >>  common/encrypt    | 12 ++++++++++++
> > >>  tests/generic/399 |  3 +--
> > >>  2 files changed, 13 insertions(+), 2 deletions(-)
> > >>
> > >>diff --git a/common/encrypt b/common/encrypt
> > >>index a6fd89d..189c59e 100644
> > >>--- a/common/encrypt
> > >>+++ b/common/encrypt
> > >>@@ -81,6 +81,18 @@ _scratch_mkfs_encrypted()
> > >>  	esac
> > >>  }
> > >>+_scratch_mkfs_sized_encrypted()
> > >>+{
> > >>+	case $FSTYP in
> > >>+	ext4|f2fs)
> > >>+		MKFS_OPTIONS="$MKFS_OPTIONS -O encrypt" _scratch_mkfs_sized $*
> > >>+		;;
> > >
> > >This does not need to screw around with MKFS_OPTIONS. This:
> > >
> > >		_scratch_mkfs_sized -O encrypt $*
> > >
> > >Will do just fine.
> > 
> > Hmm, I don't see how that could work. At the moment,
> > _scratch_mkfs_sized only takes and uses two arguments, one of which
> > is optional. AFAICS, all additional mkfs options need to be passed
> > using MKFS_OPTIONS to _scratch_mkfs_sized.
> 
> Oh, I was under the impression that got fixed some time ago.
> Screwing with MKFS_OPTIONS means defeats some of the test specific
> mkfs option conflict resolution that some filesystem have.
> 
> i.e. when the options specified by the test cause problems with test
> run specified MKFS_OPTIONS, the MKFS_OPTIONS get dropped and just
> the test specific options are used. Setting random test options in
> MKFS_OPTIONS can cause _scratch_mkfs_sized to not use the options
> specified by the test at all...

JFYI, that was fixed in the _scratch_mkfs helper, but not the _sized
one, the fs size handling makes it special and not work well with the
generic mkfs helpers.

Thanks,
Eryu