Re: [PATCH v3 2/4] selftests/landlock: Selftests for file truncation support

["Günther Noack" <gnoack3000@gmail.com>]

On Sat, Aug 13, 2022 at 02:45:12PM +0200, Mickaël Salaün wrote:
> On 13/08/2022 12:07, Günther Noack wrote:
> > On Thu, Aug 11, 2022 at 06:59:38PM +0200, Mickaël Salaün wrote:
> > > On 04/08/2022 21:37, Günther Noack wrote:
> > > > +/* Invokes creat(2) and returns the errno or 0. */
> > > > +static int test_creat(struct __test_metadata *const _metadata,
> > > > +		      const char *const path, mode_t mode)
> > > > +{
> > > > +	int fd = creat(path, mode);
> > > > +
> > > > +	if (fd < 0)
> > > > +		return errno;
> > > > +	ASSERT_EQ(0, close(fd));
> > >
> > > test_creat() contains an ASSERT, which would only print this line, which
> > > would not help much if it is called multiple times, which is the case. I
> > > prefer not passing _metadata but only returning errno or 0 and handling the
> > > ASSERT in layout1.truncate* . It is not the case everywhere but we should
> > > try to follow this rule as much as possible.
> >
> > Thanks, that's a good point that the line number attribution becomes confusing.
> >
> > I changed these ASSERT_EQ() calls to just return the errno directly.

Brief follow up on this; I changed it to return a special error EBADFD
in the places in the test_foo() helpers where I previously used
ASSERT_EQ. The errors checked there are errors from open() and close()
in cases where they are needed as prerequisites or cleanups to the
actual thing we want to test.

Specifically open() can also return EACCES due to Landlock policies,
and I don't want to confuse that with the EACCES that ftruncate() may
return.

I use EBADFD because it's a reasonably obscure error code and should
not overlap. (Suggestions welcome)

> > **To make a constructive proposal**:
> >
> > I think that using EXPECT improves debuggability in case of a test
> > failure, but both with EXPECT and with ASSERT, the tests will give the
> > same guarantee that the code works, so I feel that this should not be
> > blocking the truncate patch... so I'd just go and convert it all to
> > ASSERTs, it'll be consistent with the surrounding tests, and we can
> > have that EXPECT/ASSERT discussion separately if you like. Does that
> > sound good?
>
> You did the work to differentiate EXPECT from ASSERT, and as long as failing
> an EXPECT doesn't change the semantic of the next tests (i.e. not changing a
> common state, e.g. by creating or removing a file), I think we should keep
> your current code. This may be tricky to do correctly, hence my reluctance.
> I'll think a bit more about that but it will be much more easier to replace
> EXPECT with ASSERT than to re-add EXPECT, and it wouldn't require another
> patch series, so let's not change your patch. :)

Ack, thanks :)

--