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

[Li Wang <li.wang@linux.dev>]

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

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

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

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

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

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.

-- 
Regards,
Li Wang

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