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

[Petr Vorel <pvorel@suse.cz>]

Hi Li,

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

Description: [v3] lib: Introduce tst_path_macros.h to consolidate system paths
Applying: lib: Introduce tst_path_macros.h to consolidate system paths
error: patch failed: testcases/kernel/syscalls/newuname/newuname01.c:31
error: testcases/kernel/syscalls/newuname/newuname01.c: patch does not apply

Which is strange, I'm trying to apply on 1e619620eda111cbd94672dd5a0e1cc92567b15d~
(prior Cyril's changes to newuname01.c).

Ideally your for always had rebased version.

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).

> We have a lot of hardcoded strings and duplicated macros for /proc
> and /sys paths scattered throughout the testcases.

> This patch introduces a new common header, tst_path_macros.h, to centralize
> and standardize these path definitions. By unifying these macros, we
> reduce code duplication, improve maintainability, and ensure consistent
> path usage across the entire LTP codebase.

> Signed-off-by: Li Wang <li.wang@linux.dev>
> ---

> Notes:
>     v3:
>     	* Rename to tst_path_macros.h to reflect its macro-only content.

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:").

>     	* 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?

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

Kind regards,
Petr

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