Re: [LTP] [PATCH 3/4] fanotify24: Add test for FAN_PRE_ACCESS and FAN_DENY_ERRNO

[Petr Vorel <pvorel@suse.cz>]

> On Tue, Feb 11, 2025 at 8:09 PM Petr Vorel <pvorel@suse.cz> wrote:

> > > On Mon, Feb 10, 2025 at 4:43 PM Jan Kara <jack@suse.cz> wrote:

> > > > On Mon 10-02-25 16:13:15, Amir Goldstein wrote:
> > > > > Fork the test fanotify24 from test fanotify03, replacing the
> > > > > permission event FAN_ACCESS_PERM with the new pre-content event
> > > > > FAN_PRE_ACCESS.

> > > > > The test is changed to use class FAN_CLASS_PRE_CONTENT, which is
> > > > > required for FAN_PRE_ACCESS and this class also enabled the response
> > > > > with cutomer error code FAN_DENY_ERRNO.

> > > > > Unlike FAN_ACCESS_PERM, FAN_PRE_ACCESS is also created on write()
> > > > > system call.  The test case expected results are adjusted to
> > > > > respond with the default error (EPERM) to open() and write() and
> > > > > to respond with custom errors (EIO, EBUSY) to read() and execve().

> > > > > Not all fs support pre-content events, so run on all filesystems
> > > > > to excercise FAN_PRE_ACCESS on all supported filesystems.

> > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>

> > > > Looks good to me. I was just wondering whether some bits like
> > > > generate_events(), mark setup, child setup, main test loop could not be
> > > > factored out into a helper functions used by both old and new tests?

> > > Yes, I agree that forking the tests is bad and that we need much
> > > more common helpers.

> > > IIUC, LTP developers are going to try to come up with some proposals
> > > for refactoring helpers to split some large fanotify tests [1][2].

> > > My opinion is that factoring out helpers that are useful only for
> > > fanotify03,fanotify24 is suboptimal and we need to see if we can
> > > create much more generic helpers that could be shared by more tests.

> > > BTW, if you look closer, you will see that generate_events() is quite
> > > different between fanotify03 and fanotify24, although it is true that
> > > fanotify24 has a more generalized version that follows the expected
> > > events more closely.

> > > I did start with extending fanotify03 before I forked it and before the
> > > fork generate_events() was even more hard to follow because of
> > > the difference in expected events for write() between permission
> > > and pre-content events.

> > I'm not happy that tests are nearly identical, but agree that merging them would
> > make readability even harder. Also if there is really no value to run
> > FAN_ACCESS_PERM (fanotify03.c) on all filesystems it would prolong testing.

> > The only downside of not factoring out common code is that fix in one test will
> > not appear in the other test (it looks to me from what you wrote that you
> > improved generate_events() for fanotify24.c but not for fanotify03.c. Also now
> > is for me to remember remove else if (errno == exp_errno) [1] also for
> > fanotify24.c).

> Please don't remove these else from fanotify24.
> I meant that the else in fanotify03.c are not needed because they came

> from the generic generate_events() of fanotify24.c which supports
> different expected errno values (FAN_DENY_ERRNO(xxx)).
> fanotify03 does not have FAN_DENY_ERRNO(xxx), so comparing exp_errno
> is not needed in fanbotify03. It is needed in fanotify24.

Ah, sure, I'll keep them.

> > We have long history of this in LTP.  But I'm also not sure if
> > it's worth factoring out code just for 2 tests. We might reconsider factoring
> > out, but unless Martin or Cyril objects I would keep it for now.

> > I was looking briefly for code which could be turned into more generic helpers,
> > but so far I haven't found anything. Maybe you have better idea.


> I do not have a better idea.
> fanotify03 and fanotify24 are sufficiently similar to dup a lot of code
> and sufficiently different to make common code hard or more
> complex.

> IMO, the code that is relatively common to many fanotify tests is the
> event read and process loop.

> I think it should be doable to make these loops use common helpers for
> reading and verifying the expected events, but it is not a small job to make
> those helpers generic enough to cater all the different tests that check
> different event formats.

Thanks for a hint. I'll see if I find a time and come up with something useful.

We briefly talk with Cyril about this and agree that we don't want to block
this. I'll wait little longer if Cyril wants to add something and merge during
tomorrow. Again, thanks a lot for updating fanotify testcases in LTP.

Kind regards,
Petr

> Thanks,
> Amir.

> > FYI there is not only test code and test output readability, but also ability to
> > filter out certain fix. If particular fix is not backported to enterprise kernel
> > (WONTFIX). With one of many tests fails we have no way to distinguish it in the
> > current code (tst_kvercmp2() does not always help). Therefore we prefer to have
> > regression tests in separate files (don't mix them with tests for basic
> > functionality). But that would probably lead to even more code duplicity.

> > I was thinking whether we could try allow to run only subset of struct tcase
> > items, based on getopt parameter. E.g. run only "inode" marks (first two items),
> > or mount marks (3rd and 4th), ...  runtest/syscalls would get more items of
> > particular test. It would prolong testing (for all_filesystems quite significantly)
> > but besides better test output readability + allow to filter out fixes.

> > Kind regards,
> > Petr

> > [1] https://lore.kernel.org/ltp/CAOQ4uxgu16dOsU4uuq66CGqXw6wY8c8jK7sL1QheB8kTPU=X+g@mail.gmail.com/

> > > Thanks,
> > > Amir.

> > > [1] https://lore.kernel.org/ltp/71d4414b-802f-4019-8527-e8886e2d1aeb@suse.cz/
> > > [2] https://lore.kernel.org/ltp/20250131164217.GA1135694@pevik/

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp