[LTP] [RFC 1/3] syscalls/fanotify03: defined additional tcase members to support more tcase control

[Matthew Bobrowski <mbobrowski@mbobrowski.org>]

On Wed, Oct 24, 2018 at 08:23:47AM +0300, Amir Goldstein wrote:
> On Wed, Oct 24, 2018 at 6:27 AM Matthew Bobrowski
> <mbobrowski@mbobrowski.org> wrote:
> >
> > * Included the event mask member to be used for fanotify_mark() within
> >   the tcase. This allows control over what event types are to be
> >   generated on a tcase by tcase level as oppose to having it globally
> >   defined for all tcases.
> >
> > * Included the event_set and event_resp arrays into a tcase so that
> >   the sequence of expected events can be defined on a test case by test
> >   case level. Both event_set and event_resp are now represented by the
> >   event struct.
> >
> > * Added event_count member to tcase in order to represent the number of
> >   expected events for each tcase.
> >
> > * Fanotify permission events cannot be merged, thus cleaned up some
> >   unnecessary code in test_fanotify() that checks whether any additional
> >   bits are left within event->mask.
> >
> > Signed-off-by: Matthew Bobrowski <mbobrowski@mbobrowski.org>
> > ---
> 
> Looks good! nits below.
> 
> >  .../kernel/syscalls/fanotify/fanotify03.c     | 90 ++++++++++---------
> >  1 file changed, 50 insertions(+), 40 deletions(-)
> >
> > diff --git a/testcases/kernel/syscalls/fanotify/fanotify03.c b/testcases/kernel/syscalls/fanotify/fanotify03.c
> > index 5c105ed32..cca15aa00 100644
> > --- a/testcases/kernel/syscalls/fanotify/fanotify03.c
> > +++ b/testcases/kernel/syscalls/fanotify/fanotify03.c
> > @@ -42,28 +42,48 @@ static volatile int fd_notify;
> >
> >  static pid_t child_pid;
> >
> > -static unsigned long long event_set[EVENT_MAX];
> > -static unsigned int event_resp[EVENT_MAX];
> > -
> >  static char event_buf[EVENT_BUF_LEN];
> >  static int support_perm_events;
> >
> > +struct event {
> > +       unsigned long long type;
> 
> It's better to name this also 'mask' as in the near future you will
> want to assign it the value of OPEN_PERM|OPEN_EXEC_PERM
> which is not a 'type'.
> 
Yes, I also agree.

> > +       unsigned int response;
> > +};
> > +
> >  static struct tcase {
> >         const char *tname;
> >         struct fanotify_mark_type mark;
> > +       unsigned long long mask;
> > +       int event_count;
> > +       struct event event_set[EVENT_MAX];
> 
> Better change the value of EVENT_MAX...
> or even decouple it from EVENT_BUF_LEN
> 
Good point. OK, providing that's the case, I think doing something along
the lines of the below would be reasonable? I'm under the assumption that
this is what you meant by changing the value?

#define EVENT_SET_SIZE	(sizeof (struct event) * 16)

> >  } tcases[] = {
> >         {
> >                 "inode mark permission events",
> >                 INIT_FANOTIFY_MARK_TYPE(INODE),
> > +               FAN_OPEN_PERM | FAN_ACCESS_PERM, 2,
> > +               {
> > +                       {FAN_OPEN_PERM, FAN_ALLOW},
> > +                       {FAN_ACCESS_PERM, FAN_DENY}
> > +               }
> >         },
> >         {
> >                 "mount mark permission events",
> >                 INIT_FANOTIFY_MARK_TYPE(MOUNT),
> > +               FAN_OPEN_PERM | FAN_ACCESS_PERM, 2,
> > +               {
> > +                       {FAN_OPEN_PERM, FAN_ALLOW},
> > +                       {FAN_ACCESS_PERM, FAN_DENY}
> > +               }
> >         },
> >         {
> >                 "filesystem mark permission events",
> >                 INIT_FANOTIFY_MARK_TYPE(FILESYSTEM),
> > -       },
> > +               FAN_OPEN_PERM | FAN_ACCESS_PERM, 2,
> > +               {
> > +                       {FAN_OPEN_PERM, FAN_ALLOW},
> > +                       {FAN_ACCESS_PERM, FAN_DENY}
> > +               }
> > +       }
> >  };
> >
> >  static void generate_events(void)
> > @@ -146,8 +166,7 @@ static int setup_mark(unsigned int n)
> >
> >         fd_notify = SAFE_FANOTIFY_INIT(FAN_CLASS_CONTENT, O_RDONLY);
> >
> > -       if (fanotify_mark(fd_notify, FAN_MARK_ADD | mark->flag,
> > -                         FAN_ACCESS_PERM | FAN_OPEN_PERM,
> > +       if (fanotify_mark(fd_notify, FAN_MARK_ADD | mark->flag, tc->mask,
> >                           AT_FDCWD, fname) < 0) {
> >                 if (errno == EINVAL && support_perm_events &&
> >                     mark->flag == FAN_MARK_FILESYSTEM) {
> > @@ -167,7 +186,7 @@ static int setup_mark(unsigned int n)
> >                 }
> >         } else {
> >                 /*
> > -                * To distigouish between perm event not supported and
> > +                * To distinguish between perm event not supported and
> >                  * filesystem mark not supported.
> >                  */
> >                 support_perm_events = 1;
> > @@ -179,32 +198,22 @@ static int setup_mark(unsigned int n)
> >
> >  static void test_fanotify(unsigned int n)
> >  {
> > -       int tst_count;
> >         int ret, len = 0, i = 0, test_num = 0;
> > +       struct tcase *tc = &tcases[n];
> > +       struct event *event_set = tc->event_set;
> >
> >         if (setup_mark(n) != 0)
> >                 return;
> >
> >         run_child();
> >
> > -       tst_count = 0;
> > -
> > -       event_set[tst_count] = FAN_OPEN_PERM;
> > -       event_resp[tst_count++] = FAN_ALLOW;
> > -       event_set[tst_count] = FAN_ACCESS_PERM;
> > -       event_resp[tst_count++] = FAN_DENY;
> > -
> > -       /* tst_count + 1 is for checking child return value */
> > -       if (TST_TOTAL != tst_count + 1) {
> > -               tst_brk(TBROK,
> > -                       "TST_TOTAL and tst_count do not match");
> > -       }
> > -       tst_count = 0;
> > -
> >         /*
> > -        * check events
> > +        * Process events
> > +        *
> > +        * tc->count + 1 is to accommodate for checking the child process
> > +        * return value
> >          */
> > -       while (test_num < TST_TOTAL && fd_notify != -1) {
> > +       while (test_num < tc->event_count + 1 && fd_notify != -1) {
> >                 struct fanotify_event_metadata *event;
> >
> >                 if (i == len) {
> > @@ -222,12 +231,12 @@ static void test_fanotify(unsigned int n)
> >                 }
> >
> >                 event = (struct fanotify_event_metadata *)&event_buf[i];
> > -               if (!(event->mask & event_set[test_num])) {
> > +               if (!(event->mask & event_set[test_num].type)) {
> 
> /* Permission events cannot be merged, so... */
> +               if (event->mask != event_set[test_num].mask) {
> 
Updated.

-- 
Matthew Bobrowski <mbobrowski@mbobrowski.org>