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

Ari Sundholm
2017-12-21T22:17:46Z

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