| Message ID | 1513792000-25462-1-git-send-email-ari@tuxera.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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 -- 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
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 -- 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
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.
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 -- 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
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 -- 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 --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)
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(-)