[RFC PATCH v1 03/10] selftests/landlock: Create 'create' test

[Mickaël Salaün]

On Thu, May 16, 2024 at 04:54:58PM +0300, Ivanov Mikhail wrote:
> 
> 
> 5/8/2024 1:38 PM, Mickaël Salaün wrote:
> > On Thu, Apr 11, 2024 at 06:58:34PM +0300, Ivanov Mikhail wrote:
> > > 
> > > 4/8/2024 4:08 PM, Günther Noack wrote:


> > > > > +		{
> > > > > +			TH_LOG("Failed to create socket: %s", strerror(errno));
> > > > > +		}
> > > > > +		EXPECT_EQ(0, close(fd));
> > > > > +	}
> > > > > +}
> > > > 
> > > > This is slightly too much logic in a test helper, for my taste,
> > > > and the meaning of the true/false argument in the main test below
> > > > is not very obvious.
> > > > 
> > > > Extending the idea from above, if test_socket() was simpler, would it
> > > > be possible to turn these fixtures into something shorter like this:
> > > > 
> > > >     ASSERT_EQ(EAFNOSUPPORT, test_socket(AF_UNSPEC, SOCK_STREAM, 0));
> > > >     ASSERT_EQ(EACCES, test_socket(AF_UNIX, SOCK_STREAM, 0));
> > > >     ASSERT_EQ(EACCES, test_socket(AF_UNIX, SOCK_DGRAM, 0));
> > > >     ASSERT_EQ(EACCES, test_socket(AF_INET, SOCK_STREAM, 0));
> > > >     // etc.
> > 
> > I'd prefer that too.
> > 
> > > > 
> > > > Would that make the tests easier to write, to list out the table of
> > > > expected values aspect like that, rather than in a fixture?
> > > > 
> > > > 
> > > 
> > > Initially, I conceived this function as an entity that allows to
> > > separate the logic associated with the tested methods or usecases from
> > > the logic of configuring the state of the Landlock ruleset in the
> > > sandbox.
> > > 
> > > But at the moment, `test_socket_create()` is obviously a wrapper over
> > > socket(2). So for now it's worth removing unnecessary logic.
> > > 
> > > But i don't think it's worth removing the fixtures here:
> > > 
> > >    * in my opinion, the design of the fixtures is quite convenient.
> > >      It allows you to separate the definition of the object under test
> > >      from the test case. E.g. test protocol.create checks the ability of
> > >      Landlock to restrict the creation of a socket of a certain type,
> > >      rather than the ability to restrict creation of UNIX, TCP, UDP...
> > >      sockets
> > 
> > I'm not sure to understand, but we need to have positive and negative
> > tests, potentially in separate TEST_F().
> 
> I mean we can use fixtures in order to not add ASSERT_EQ for
> each protocol, as suggested by Günther. It's gonna look like this:
> 
>      ASSERT_EQ(EAFNOSUPPORT, test_socket(&self->unspec_srv0));
>      ASSERT_EQ(EACCES, test_socket(&self->srv0));
> 
> I think it would make the test easier to read, don't you think so?

Yes, this looks good.