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

[Li Wang]

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.

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

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



>
> > @@ -1352,3 +1352,19 @@ int safe_sysinfo(const char *file, const int
> lineno, struct sysinfo *info)
>
> >       return ret;
> >  }
> > +
> > +int safe_print_file(const char *file, const int lineno, char *path)
> > +{
> > +     int ret;
> > +     FILE *pfile;
> > +     char line[PATH_MAX];
> > +
> > +     pfile = safe_fopen(file, lineno, NULL, path, "r");
> > +
> > +     while (fgets(line, sizeof(line), pfile))
> > +             tst_resm_(file, lineno, TINFO, "%s", line);
> > +
> > +     ret = safe_fclose(file, lineno, NULL, pfile);
> > +
> > +     return ret;
> > +}
> > diff --git a/lib/tst_memutils.c b/lib/tst_memutils.c
> > index c5382ff10..6c1e7c29e 100644
> > --- a/lib/tst_memutils.c
> > +++ b/lib/tst_memutils.c
> > @@ -182,3 +182,8 @@ void tst_disable_oom_protection(pid_t pid)
> >  {
> >       set_oom_score_adj(pid, 0);
> >  }
> > +
> > +void tst_print_meminfo_(const char *file, const int lineno)
> > +{
> > +     safe_print_file(file, lineno, "/proc/meminfo");
> As I mentioned above, we try to avoid function wrappers.
> > +}
>
>

-- 
Regards,
Li Wang