On Mon, Nov 21, 2016 at 10:40:50AM -0800, Eric Biggers wrote:
> Hi Dave, thanks for reviewing.
>
> On Mon, Nov 21, 2016 at 08:33:13AM +1100, Dave Chinner wrote:
> > > +
> > > +. ./common/rc
> >
> > Tests will already have included common/rc before this file, so we
> > do not source it here.
> ...
> > These go in the tests, not here.
> ...
> > _requires_real_encryption()
> >
> > In each test.
> ...
> > And this should all be in a _requires_encryption() function.
> >
>
> I'll do all of these. Of course the intent was to avoid duplicating code in
> each test, but I will use the more verbose style if that's preferred. I assume
> you'd also prefer explicitly formatting and mounting the scratch device in each
> test even though _require_encryption would already have to do that?
Yes, because in future _require_encryption may change to no require
touching the scratch device. Also, checking for encryption may
create a filesystem with different configuration than the one we
want to test. So it's better to be consistent across all tests and
require the scratch_mkfs call in each test so we know the exact
state of the filesystem before the test starts....
> > Ok, can we get this added to xfs_io rather than a stand-alone
> > fstests helper? There are three clear commands here:
> >
> > {"gen_key", gen_key},
> > {"rm_key", rm_key},
> > {"set_policy", set_policy},
> >
> > So it should plug straight into the xfs_io command parsing
> > infrastructure without much change at all.
>
> I see that xfs_io is part of xfsprogs, not xfstests. Does it make sense to add
> filesystem encryption commands to xfs_io even though XFS doesn't support them
> yet? Currently only ext4 and f2fs support filesystem encryption via this common
> API. (ubifs support has been proposed too.)
Yes, because it is in the plan to support the generic encryption in
XFS, too. It'll just take us a little while to get to it, but it
won't hurt to support these operations ahead of that time...
> If we do go that route then it should be considered only adding
> "set_policy" and "get_policy" commands, and for "gen_key" and
> "rm_key" instead using shell wrappers around 'keyctl' instead.
> gen_key and rm_key don't touch the filesystem at all; they only
> work with the keyring. It's possible to use 'keyctl padd' to add
> a fscrypt key, though it's not completely trivial because you'd
> have to use 'echo -e' to generate the C structure 'struct
> fscrypt_key' with mode = 0, raw = actual key in binary, size = 64.
Sounds fine to me.
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
