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

[Petr Vorel <pvorel@suse.cz>]

> Petr Vorel wrote:

> > Hi Li,

> > could you please rebase to master and push branch to your fork?
> > Patch now does not apply:

> Sure, rebased and pushed to my fork:
>  https://github.com/wangli5665/ltp/tree/tst_path_defs

Thank you!

+1. I'll try to find time soon, but probably after SUSE Labs conference
(which is next week till Thursday).

> > I suppose metadata will work with it correctly since Cyril's support for macros
> > in past (i.e. metadata/ltp.json will have "/proc/sys/kernel/hostname", not
> > PATH_HOSTNAME, which tells nothing to LTP users).

> Oh, I didn't realized this, seems the ltp.json contains define words:

> # cat metadata/ltp.json |grep PATH_
>      "PATH_MAX_USER_NAMESPACES",
>      "PATH_MAX_USER_NAMESPACES",
>      "PATH_MAX_USER_NAMESPACES",
>      "PATH_MAX_USER_NAMESPACES",
>      "PATH_UNPRIVILEGED_USERNS_CLONE",
>      "PATH_MAX_USER_NAMESPACES",
>      "PATH_UNPRIVILEGED_USERNS_CLONE",
>      "PATH_VM_OVERCOMMIT_HPAGES",
>      "PATH_VM_OVERCOMMIT_HPAGES",
>      ...

> I can change them back to the original types (for ltp.json) in a separate patch.

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.

> > I slightly prefer the previous name tst_path.h (for me macros are complex
> > functions, not just simple definitions, what info gives me "macro-only content"?
> > I care more about the actual content). I'm ok with it current name, but maybe
> > kernel_paths.h or proc_sys_paths.h would be more obvious which paths is it
> > about. Also, it's not just about paths, but also about the content in these
> > /proc|sys files (e.g. "HugePages_Total:").

> Yes, I'm also hesitant about the naming. The tst_ prefix is needed since it will
> be placed in the include/ directory and used across the entire project. proc_ or
> sys_ sounds a bit too narrow in scope. Maybe:

>   tst_path_defs.h
>   tst_sys_paths.h

> I'm now leaning towards tst_path_defs.h. WDYT?

I'm OK with it. My guess is that it will sooner or later contain more than just
paths (it already has now /proc/meminfo keys) but I'm not able to suggest
anything better.

I was also thinking about name containing "uapi", because it's all about
info provided by kernel to userspace (yes, mostly paths but not only), but that
would lead to confusions about kernel's uapi (i.e. what we more or less have as
lapi/).

> > >     	* Replaced generic string concatenation macros with explicit.
> > >     	* Manualy built & Tested all touched tests on my c10s vm.

> > ...

> > > +/* KERNEL */
> > > +#define PATH_HOSTNAME			"/proc/sys/kernel/hostname"
> > > +#define PATH_OSRELEASE			"/proc/sys/kernel/osrelease"
> > ...
> > > +/* USER */
> > > +#define PATH_MAX_USER_NAMESPACES	"/proc/sys/user/max_user_namespaces"

> > I have 2 concerns:

> > 1) we often try to have LTP specific 'TST_' prefix to avoid clash with
> > whatever definition. Is it ok to ignore it now?

> I'd prefer to stick with PATH_ instead of TST_PATH_. PATH_ is universally
> understood as a file path definition. Adding TST_ makes it unnecessarily
> verbose.

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.

> > 2) I'm pretty sure I will have to look into this file often, because from
> > definition name I have no idea where exactly file is. I'd be ok with long name
> > TST_PROC_SYS_KERNEL_HOSTNAME, but understand Cyril wants short names. Maybe at
> > least TST_PATH_KERNEL_HOSTNAME and TST_PATH_USER_MAX_USER_NAMESPACES(last
> > directory and file)? That would be similar to what sysctl is using:

> > 	sysctl kernel.hostname
> > 	sysctl user.max_user_namespaces

> I understand the concern, but long macros like TST_PATH_USER_MAX_USER_NAMESPACES
> will make the test code very verbose and easily lead to 80-character
> line wrapping issues.

Yeah, true.

> To address the readability issue without making the names too long,
> I can add _KERNEL_ and _USER_ to the defines (like PATH_KERNEL_HOSTNAME
> and PATH_USER_MAX_USER_NAMESPACES). I think this is a good middle ground.

Sounds good to me.

Kind regards,
Petr

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