Re: [LTP] [PATCH v3 1/2] tst_memutils.c: Add tst_print_meminfo function

[Petr Vorel <pvorel@suse.cz>]

Hi Li, all,

> Hi Petr, All,

> On Sat, Dec 16, 2023 at 2:58 AM Petr Vorel <pvorel@suse.cz> wrote:

> > Hi Wei,

> > ...
> > > +++ b/include/tst_memutils.h
> > > @@ -58,4 +58,10 @@ void tst_enable_oom_protection(pid_t pid);
> > >   */
> > >  void tst_disable_oom_protection(pid_t pid);

> > > +void tst_print_meminfo(void);
> > > +
> > > +void tst_print_meminfo_(const char *file, const int lineno);
> > > +
> > > +#define tst_print_meminfo() tst_print_meminfo_(__FILE__, __LINE__)
> > Most of the macros we have upper case, can it be please
> > TST_PRINT_MEMINFO() ?
> > I guess it does not have to be SAFE_PRINT_MEMINFO().

> > And because it's just one liner, could it be:

> > #define TST_PRINT_MEMINFO() safe_print_file(__FILE__, __LINE__,
> > "/proc/meminfo")

> > ...

> > > +++ b/lib/safe_macros.c

> > We don't want to add anything to the legacy API (otherwise it would go to
> > lib/safe_file_ops.c), please add this to lib/tst_safe_macros.c.

> > BTW I'm slightly confused, what would be the best place for this,
> > lib/tst_safe_macros.c is being used nowadays for everything. But there is
> > also



> > include/tst_safe_file_ops.h, which does not have C file
> > (lib/tst_safe_file_ops.c) because it internally use lib/tst_safe_macros.c.


> No, basically it does not use the lib/tst_safe_macros.c.

You're right.

> From what I understand, 'tst_safe_file_ops.h' is just a header for
> collecting
> all the file operations for Linux, it doesn't touch other subcomponent ops.

Thanks! Now it's obvious.

> 'tst_safe_file_ops.h' defines macros for all functions in
> 'safe_file_ops_fn.h'
> and archived them in 'safe_file_ops.c' lib.

> Something like this combination:

> tst_safe_file_ops.h:
>     safe_file_ops_fn.h
>     safe_file_ops.c

> tst_safe_macros.h
> tst_safe_macros.c



> > I guess creating lib/tst_safe_macros.c was postponed until we rewrite all
> > tests,
> > maybe it's a time to create it.




> > @Li @Cyril: Also include/tst_safe_file_ops.h has SAFE_READ_MEMINFO() and
> > SAFE_READ_PROC_STATUS(), IMHO these should be in include/tst_memutils.h.
> > Or, we shouldn't have 2 headers for similar thing, it would be good to
> > merge
> > these two.


> Agreed, anything related to the dedicated ops should be put into the
> corresponding header files. tst_safe_file_ops.h is a generic operation
> for Linux (but not for specific) files. So I vote for adding *_MEMINFO()
> to tst_memutil.h.

+1

I understand that it's a good idea when we separate things according to e.g.
libc header. But we also have separated C files in lib/.
It's probably easier if we have more shorter files than fewer very long files,
but I wonder if some sourcers should not be in single files, e.g. these:
tst_supported_fs_types.c, tst_fs_type.c => tst_fs.c
or
tst_fill_fs.c, tst_fill_file.c => tst_fill.c
or
tst_fs_setup.c, tst_path_has_mnt_flags.c => tst_mount.c
(into some more generic name)

Nothing critical, it just having 1-3 functions in separate source makes actually
search harder, because file name is very specific

Kind regards,
Petr

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