Re: [LTP] [PATCH v3] lib: Introduce tst_path_macros.h to consolidate system paths

[Petr Vorel <pvorel@suse.cz>]

Hi Li,

> Petr Vorel wrote:

> > The problem is even now on master:
> > output of testcases/kernel/mem/hugetlb/hugemmap/hugemmap10.c contains e.g. PATH_OC_HPAGES
> > from include/tst_hugepage.h.

> > https://linux-test-project.readthedocs.io/en/latest/users/test_catalog.html#hugemmap10

> > da3088183a ("metadata: metaparse: Implement recursive include")
> > skips certain LTP includes e.g. tst_test.h in skip_includes[] causes
> > all definitions not being propagated. I guess it could be fixable to parse
> > include/tst_test.h and include certain headers.

> > $ metadata/metaparse -v testcases/kernel/mem/hugetlb/hugemmap/hugemmap10.c
> > does not print INCLUDE .. because include/tst_hugepage.h is included via
> > tst_test.h, not directly in the test.

> After looking into the mataparse.c I think we have two methods:

> 1. Explicitly parsing tst_path_defs.h in metaparse.c:

>    Since tst_test.h is explicitly ignored by the skip_includes[] array,
>    any files it includes (e.g. tst_path_defs.h) are consequently bypassed
>    by the parser. To resolve this, we can instruct metaparse to proactively
>    search for and parse tst_path_defs.h during its initialization phase.

>    We can achieve this by adding a dedicated function in metaparse.c to
>    read this specific header file, and then invoking it within the main
>    parse_file function.

LGTM.

> 2. Simply adding the list of macros to the internal_macros[].

>    As long as all the konbs definitions are listed there, metaparse can
>    convert them into string paths in tst_test for use in ltp.json.

> --- a/metadata/metaparse.c
> +++ b/metadata/metaparse.c
> @@ -881,6 +881,66 @@ static const struct macro {
>         {"TST_SR_TBROK_RO", "TBROK_RO"},
>         {"TST_SR_TCONF_RO", "TCONF_RO"},
>         {"TST_SR_SKIP_RO", "SKIP_RO"},
> +        {"PATH_KERNEL_HOSTNAME", "/proc/sys/kernel/hostname"},
> +        {"PATH_KERNEL_OSRELEASE", "/proc/sys/kernel/osrelease"},
> +        {"PATH_KERNEL_VERSION", "/proc/sys/kernel/version"},
> +        {"PATH_KERNEL_DOMAINNAME", "/proc/sys/kernel/domainname"},
> +        {"PATH_KERNEL_PRINTK", "/proc/sys/kernel/printk"},
> +        {"PATH_KERNEL_PID_MAX", "/proc/sys/kernel/pid_max"},
> +        {"PATH_KERNEL_SHMMAX", "/proc/sys/kernel/shmmax"},
> ...

If you mean that the definitions would be on more places (tst_path_defs.h and
metadata/metaparse.c) I would not consider is as a good option.

> I don't have any preferences, but method-1 looks more elegant.

+1

> > My fear was that to clash with existing system headers causing build failure due
> > macro redefinition (that's IMHO reason for "TST_" prefix). But "PATH_" might
> > be safe, because /usr/include/paths.h contains "_PATH_" prefix - with leading
> > underscore (e.g. _PATH_CONSOLE). And the only "PATH_" prefix is PATH_MAX
> > in /usr/include/limits.h.

> We now have PATH_{KERNEL|USER|VM|FS}, so conflicts should be very low.

OK with me, but I'd prefer to get this acked by somebody else.

Kind regards,
Petr

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