Message-ID: <ychkzjlukpzb4h24dxtvesnvq3tgjrtqcfed6xlorzpy24xk43@zdips4bvrsii>
Date:   Wed, 28 Jun 2023 18:03:26 +0200
From:   Ahelenia Ziemiańska 
        <nabijaczleweli@...ijaczleweli.xyz>
To:     Amir Goldstein <amir73il@...il.com>
Cc:     Alexander Viro <viro@...iv.linux.org.uk>,
        Christian Brauner <brauner@...nel.org>,
        linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
        Jan Kara <jack@...e.cz>,
        Chung-Chiang Cheng <cccheng@...ology.com>, ltp@...ts.linux.it,
        Petr Vorel <pvorel@...e.cz>
Subject: Re: [LTP RFC PATCH v3] inotify13: new test for fs/splice.c functions
 vs pipes vs inotify

On Wed, Jun 28, 2023 at 08:30:15AM +0300, Amir Goldstein wrote:
> On Wed, Jun 28, 2023 at 3:21 AM Ahelenia Ziemiańska
> > diff --git a/testcases/kernel/syscalls/inotify/inotify13.c b/testcases/kernel/syscalls/inotify/inotify13.c
> > new file mode 100644
> > index 000000000..97f88053e
> > --- /dev/null
> > +++ b/testcases/kernel/syscalls/inotify/inotify13.c
> > @@ -0,0 +1,282 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*\
> > + * [Description]
> > + * Verify splice-family functions (and sendfile) generate IN_ACCESS
> > + * for what they read and IN_MODIFY for what they write.
> > + *
> > + * Regression test for 983652c69199 ("splice: report related fsnotify events") and
> > + * https://lore.kernel.org/linux-fsdevel/jbyihkyk5dtaohdwjyivambb2gffyjs3dodpofafnkkunxq7bu@jngkdxx65pux/t/#u
> The process of posting a test for the fix that was not yet merged
> is indeed a chicken and egg situation.
> 
> What I usually do is post a draft test (like this) and link
> to the post of the LTP test (and maybe a branch on github)
> when posting the fix, to say how I tested the fix.
https://git.sr.ht/~nabijaczleweli/ltp/commit/v4 for now.

> I would then put it in my TODO to re-post the LTP
> test once the kernel fix has been merged.
Yep.

> > +static int compar(const void *l, const void *r)
> > +{
> > +       const struct inotify_event *lie = l;
> > +       const struct inotify_event *rie = r;
> > +
> > +       return lie->wd - rie->wd;
> > +}
> > +
> > +static void get_events(size_t evcnt, struct inotify_event evs[static evcnt])
> > +{
> > +       struct inotify_event tail, *itr = evs;
> > +
> > +       for (size_t left = evcnt; left; --left)
> > +               SAFE_READ(true, inotify, itr++, sizeof(struct inotify_event));
> > +
> > +       TEST(read(inotify, &tail, sizeof(struct inotify_event)));
> > +       if (TST_RET != -1)
> > +               tst_brk(TFAIL, ">%zu events", evcnt);
> > +       if (TST_ERR != EAGAIN)
> > +               tst_brk(TFAIL | TTERRNO, "expected EAGAIN");
> > +
> > +       qsort(evs, evcnt, sizeof(struct inotify_event), compar);
> > +}
> > +
> > +static void expect_transfer(const char *name, size_t size)
> > +{
> > +       if (TST_RET == -1)
> > +               tst_brk(TBROK | TERRNO, "%s", name);
> > +       if ((size_t)TST_RET != size)
> > +               tst_brk(TBROK, "%s: %ld != %zu", name, TST_RET, size);
> > +}
> > +
> > +static void expect_event(struct inotify_event *ev, int wd, uint32_t mask)
> > +{
> > +       if (ev->wd != wd)
> > +               tst_brk(TFAIL, "expect event for wd %d got %d", wd, ev->wd);
> > +       if (ev->mask != mask)
> > +               tst_brk(TFAIL,
> > +                       "expect event with mask %" PRIu32 " got %" PRIu32 "",
> > +                       mask, ev->mask);
> > +}
> > +
> > +// write to file, rewind, transfer accd'g to f2p, read from pipe
> > +// expecting: IN_ACCESS memfd, IN_MODIFY pipes[0]
> > +static void file_to_pipe(const char *name, ssize_t (*f2p)(void))
> > +{
> > +       struct inotify_event events[2];
> > +       char buf[strlen(name)];
> > +
> > +       SAFE_WRITE(SAFE_WRITE_RETRY, memfd, name, strlen(name));
> > +       SAFE_LSEEK(memfd, 0, SEEK_SET);
> > +       watch_rw(memfd);
> > +       watch_rw(pipes[0]);
> > +       TEST(f2p());
> > +       expect_transfer(name, strlen(name));
> > +
> > +       get_events(ARRAY_SIZE(events), events);
> > +       expect_event(events + 0, 1, IN_ACCESS);
> > +       expect_event(events + 1, 2, IN_MODIFY);
> So what I meant to say is that if there are double events that
> usually get merged (unless reader was fast enough to read the
> first event), this is something that I could live with, but encoding
> an expectation for a double event, that's not at all what I meant.
> 
> But anyway, I see that you've found a way to work around
> this problem, so at least the test can expect and get a single event.
I've tried (admittedly, not all that hard) to read a double out modify
event in this case with the v4 kernel patchset and haven't managed it.

> I think you are missing expect_no_more_events() here to
> verify that you won't get double events.
get_events() reads precisely N events, then tries to read another,
and fails if that succeeds.

Maybe a better name would be "get_events_exact()".

> See test inotify12 as an example for a test that encodes
> expect_events per test case and also verifies there are no
> unexpected extra events.
> 
> That's also an example of a more generic test template,
> but your test cases are all a bit different from each other is
> subtle ways, so I trust you will find the best balance between
> putting generic parameterized code in the run_test() template
> and putting code in the test case subroutine.
Yes, that's indeed an optics issue: it looks like there's more, but
the only actually "common" bit of the test drivers is that they all
read events in the middle: the set-up before is different, and the
additional post-conditions are different.

We /could/ encode the expected events in the test array, but then
that would put the expected events away from the code that generates
them, which is more code, and more confusing for no good reason I
think.

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)

Powered by blists - more mailing lists