Received-SPF: softfail (google.com: domain of transitioning linux-ext4-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Date:   Mon, 2 May 2022 10:19:10 -0700
From:   Eric Biggers <ebiggers@kernel.org>
To:     tytso <tytso@mit.edu>
Cc:     fstests@vger.kernel.org, linux-fscrypt@vger.kernel.org,
        linux-ext4@vger.kernel.org, Lukas Czerner <lczerner@redhat.com>
Subject: Re: [xfstests PATCH 1/2] ext4/053: update the test_dummy_encryption
 tests
Message-ID: <YnASjrPbudBXCYfK@sol.localdomain>
References: <20220501051928.540278-1-ebiggers@kernel.org>
 <20220501051928.540278-2-ebiggers@kernel.org>
 <Ym/Sk7D17iCeQIJa@mit.edu>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Ym/Sk7D17iCeQIJa@mit.edu>
Precedence: bulk

On Mon, May 02, 2022 at 05:46:11AM -0700, tytso wrote:
> On Sat, Apr 30, 2022 at 10:19:27PM -0700, Eric Biggers wrote:
> > 
> > The kernel patch "ext4: only allow test_dummy_encryption when supported"
> > will tighten the requirements on when the test_dummy_encryption mount
> > option will be accepted.  Update ext4/053 accordingly.
> 
> One of the problems with ext4/053 is that it is very implementation
> dependent.  It was useful when we were making the change to the new
> mount API, but the problem is any future changes to the mount option
> handling is going to break the patch.
> 
> So for example, the kernel patch which Eric has proposed, "ext4: only
> allow test_dummy_encryption when supported", breaks ext4/053, which I
> noted in the review the patch.  But then this patch will break kernels
> as they currently stand without this patch, and for kernels that
> haven't moved to the new mount API, fixing it is going to be a mess.
> 
> Perhaps ext4/053 is still useful in that it will flag changes that
> might unintentionally make user-visible changes mount options handling
> in ext4, but for cases like this one, where we are changing a mount
> option which is really intended for kernel developers, perhaps the
> right approach here is to just remove the parts of ext4/053 that are
> testing the behaviour of test_dummy_encryption in such a
> super-nit-picky way?
> 
> What do folks think?

I'd like to keep the test_dummy_encryption test cases.  Trying to add a couple
new test cases (patch 2) actually found a regression.

We could gate them on the kernel version, similar to the whole ext4/053 which
already only runs on kernel version 5.12.  (Kernel versions checks suck, but
maybe it's the right choice for this very-nit-picky test.)  Alternatively, I
could just backport "ext4: only allow test_dummy_encryption when supported" to
5.15, which would be the only relevant LTS kernel version.

> 
> > Move the test cases to later in the file to group them with the other
> > test cases that use do_mkfs to add custom mkfs options instead of using
> > the "default" filesystem that the test creates at the beginning.
> 
> Note: this patch doesn't apply because ext4/053 currently has this
> line:
> 
> 		not_mnt test_dummy_encryption=v3
> 
> and the patch is trying to remove this line in the first patch chunk:
> 
> 		mnt test_dummy_encryption=v3 ^test_dummy_encryption=v3
> 
> I checked the upstream version of ext4/053 just in case I had some
> local modification of ext4/053 in my tree via "git diff -r
> origin/master tests/ext4/053" but that returned no deltas.
> 
> Eric, do you have a local modification to this test in your tree
> already, perhaps?

Sorry about that; as I mentioned in the cover letter, this is based on my other
patch "ext4/053: fix the rejected mount option testing".  As-is, 'not_mnt'
doesn't really work at all, so I wanted to fix that first.

- Eric