[LTP] [PATCH 3/4] Add fanotify_get_supported_init_flags() helper function

[Amir Goldstein]

On Thu, Oct 13, 2022 at 6:49 PM Martin Doucha <mdoucha@suse.cz> wrote:
>

Hi Martin,

Please refrain from empty commit messages.
The text in your cover letter would have been useful in this commit message
and in the commit message of patch 4/4.
If it were me, I would squash those two patches
to emphasise the usability aspect of this helper, but up to you.

> Signed-off-by: Martin Doucha <mdoucha@suse.cz>
> ---
>  testcases/kernel/syscalls/fanotify/fanotify.h | 20 +++++++++++++++++++
>  1 file changed, 20 insertions(+)
>
> diff --git a/testcases/kernel/syscalls/fanotify/fanotify.h b/testcases/kernel/syscalls/fanotify/fanotify.h
> index 51078103e..43434f6ac 100644
> --- a/testcases/kernel/syscalls/fanotify/fanotify.h
> +++ b/testcases/kernel/syscalls/fanotify/fanotify.h
> @@ -213,6 +213,26 @@ static inline int fanotify_init_flags_supported_by_kernel(unsigned int flags)
>         return fanotify_init_flags_supported_on_fs(flags, NULL);
>  }
>
> +/*
> + * Check support of given init flags one by one and return those which are
> + * supported.
> + */
> +static inline unsigned int fanotify_get_supported_init_flags(unsigned int flags,
> +       const char *fname)
> +{
> +       unsigned int flg, ret = 0;
> +
> +       for (flg = 1; flg; flg <<= 1) {
> +               if (!(flags & flg))
> +                       continue;
> +
> +               if (!fanotify_init_flags_supported_on_fs(flg, fname))
> +                       ret |= flg;
> +       }
> +
> +       return ret;
> +}
> +

I am afraid that you have tried to generalize too much.
As a generic helper, this is wrong code, because in many cases
init flags depend on each other, for example,
FAN_REOPRT_NAME is not supported without FAN_REPORT_FID
so the test for support on every single bit independently is plain wrong.

If the code would have checked:

    fanotify_init_flags_supported_on_fs(ret | flg, fname))

It would have least been correct for the private case on fanotify
which tests the support for the flag combination
FAN_REPORT_DFID_NAME_TARGET

But the general helper would still be incorrect.

The reason that the helper would work for fanotify14
is because the chronological order in which kernel support was
added to flags (_FID, _DIR_FID, _NAME, _TARGE_FID)
matches the order of the flag bits.

So my recommendation is not to attempt to guess the
supported flag combinations, but test the relevant flag
combinations explicitly.

1. Change the return value of fanotify_init_flags_supported_on_fs()
    to <the set of supported flags> or 0
2. Change all code if (fan_report_*_unsupported) to if (!fan_report_*_supported)
    or better yet if (tc->init_flags & ~fan_report_*_supported) as in patch 4/4
3. fanotify14 setup() code would then look like this:

-        /* Require FAN_REPORT_FID support for all tests to simplify
per test case requirements */
-        REQUIRE_FANOTIFY_INIT_FLAGS_SUPPORTED_ON_FS(FAN_REPORT_FID, MNTPOINT);
-
-       fan_report_target_fid_unsupported =
-
fanotify_init_flags_supported_on_fs(FAN_REPORT_DFID_NAME_TARGET,
MNTPOINT);
+       supported_init_flags =
fanotify_init_flags_supported_on_fs(FAN_REPORT_FID,
+               MNTPOINT);
+       supported_init_flags |=
fanotify_init_flags_supported_on_fs(FAN_REPORT_DFID_NAME,
+               MNTPOINT);
+       supported_init_flags |=
fanotify_init_flags_supported_on_fs(FAN_REPORT_TARGET_FID,
+               MNTPOINT);

Checking these combinations is sufficient and is more readable IMO.
This approach makes extending tests easy.

In the development history of fanotify14, commit 9df7f38c8
  syscalls/fanotify14: Test cases for FAN_REPORT_DFID_NAME
would have added the setup check for supported FAN_REPORT_DFID_NAME

and commit 66d406407
  syscalls/fanotify14: Add tests for FAN_REPORT_TARGET_FID and FAN_RENAME
would have added the setup check for supported FAN_REPORT_TARGET_FID

But the check for supported flags per test case would not need to change
if it was generic like in patch 4/4.

Thanks,
Amir.