Re: [LTP] [PATCH v2 1/2] ioprio: use ioprio.h kernel header if it exists

[Petr Vorel <pvorel@suse.cz>]

Hi Damien,

thanks for this effort!

> For the ioprio system call test cases, avoid blindly defining the
> IOPRIO_XXX macro internally and instead use the kernel user API header
> file if it exists. Given that the definitions in this header file have
> changed over time, make sure to test for the existence of the macro
> IOPRIO_PRIO_LEVEL macro and define it if it does not exist. Similarly,
> use IOPRIO_NR_LEVELS to define IOPRIO_PRIO_NUM if that macro exists.

> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

If I'm correct, the only change in v2 is added Linus Walleij's RBT.
nit: it'd be better if he sent it himself.

> ---
>  configure.ac                              |  1 +
>  testcases/kernel/syscalls/ioprio/ioprio.h | 29 +++++++++++++++++------
>  2 files changed, 23 insertions(+), 7 deletions(-)

> diff --git a/configure.ac b/configure.ac
> index 548288310..e4aa2cadf 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -56,6 +56,7 @@ AC_CHECK_HEADERS_ONCE([ \
>      linux/if_ether.h \
>      linux/if_packet.h \
>      linux/io_uring.h \
> +    linux/ioprio.h \
>      linux/keyctl.h \
>      linux/mempolicy.h \
>      linux/module.h \
> diff --git a/testcases/kernel/syscalls/ioprio/ioprio.h b/testcases/kernel/syscalls/ioprio/ioprio.h
> index c74380475..6ca134a54 100644
> --- a/testcases/kernel/syscalls/ioprio/ioprio.h
> +++ b/testcases/kernel/syscalls/ioprio/ioprio.h
FYI the header should be moved to include/lapi/,
but that can be done as a separate effort afterwards (by us LTP developers).

> @@ -6,6 +6,12 @@
>  #ifndef LTP_IOPRIO_H
>  #define LTP_IOPRIO_H

There needs to be
#include "config.h"

(Otherwise header will be never included.)

With this change
Reviewed-by: Petr Vorel <pvorel@suse.cz>

> +#ifdef HAVE_LINUX_IOPRIO_H
> +
> +# include <linux/ioprio.h>
> +
> +#else

NOTE: yes, we have policy to include kernel (or libc) headers in LTP LAPI
headers [1], but we usually instead of #else part always check for constants like
this:

#ifdef HAVE_LINUX_IOPRIO_H
# include <linux/ioprio.h>
#endif

#ifndef IOPRIO_CLASS_SHIFT
# define IOPRIO_CLASS_SHIFT	(13)
#endif
...
(Exactly the same way you do for e.g. IOPRIO_NR_LEVELS or IOPRIO_PRIO_LEVEL.)

I'm ok with this variant, because it's simpler (the header was added in
v5.15-rc1) and we can check for enum. But once some constant or enum gets
changed we'd need to handle this.

Kind regards,
Petr

[1] https://github.com/linux-test-project/ltp/wiki/C-Test-API#lapi-headers

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