| Message ID | 20231214071321.9863-2-wegao@suse.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add tst_print_meminfo function into swapping01 | expand |
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. 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. > @@ -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. > +}
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. > > +} > >
Li Wang <liwang@redhat.com> wrote: > >> > +++ 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 > The reason to split them into two headers is for backward compatibility. Technically we should declare 'safe_file_ops_fn.h' functions in one header 'tst_safe_file_ops.h' but you know we have 'old_safe_file_ops.h'. tst_safe_file_ops.h: safe_file_ops_fn.h safe_file_ops.c old_safe_file_ops.h: safe_file_ops_fn.h safe_file_ops.c In the future, the ideal cleanup direction is just to have: tst_safe_file_ops.h tst_safe_file_ops.c
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
> Li Wang <liwang@redhat.com> wrote: > >> > +++ 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 > The reason to split them into two headers is for backward compatibility. > Technically we should declare 'safe_file_ops_fn.h' functions in one header > 'tst_safe_file_ops.h' but you know we have 'old_safe_file_ops.h'. > tst_safe_file_ops.h: > safe_file_ops_fn.h > safe_file_ops.c > old_safe_file_ops.h: > safe_file_ops_fn.h > safe_file_ops.c > In the future, the ideal cleanup direction is just to have: > tst_safe_file_ops.h > tst_safe_file_ops.c Yep, the problem that Cyril originally implemented new API via legacy to support both APIs (good and needed solution) and now we add only into new API (also good solution) + that approach specific (and thus small) C sources initially and later more generic (and longer) makes library a bit inconsistent. I know, it will be solved once we get rid of the legacy API, but that will take time. Kind regards, Petr
Petr Vorel <pvorel@suse.cz> wrote: > > > 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) > Yes, I think so. Unless there is some necessary reason (I overlooked) to prevent mergeing them together.
diff --git a/include/tst_memutils.h b/include/tst_memutils.h index 19b593430..439b2485a 100644 --- a/include/tst_memutils.h +++ 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__) + #endif /* TST_MEMUTILS_H__ */ diff --git a/include/tst_safe_macros.h b/include/tst_safe_macros.h index c899c4f2c..520a173dd 100644 --- a/include/tst_safe_macros.h +++ b/include/tst_safe_macros.h @@ -671,4 +671,6 @@ int safe_sysinfo(const char *file, const int lineno, struct sysinfo *info); #define SAFE_SYSINFO(info) \ safe_sysinfo(__FILE__, __LINE__, (info)) +int safe_print_file(const char *file, const int lineno, char *path); + #endif /* SAFE_MACROS_H__ */ diff --git a/lib/safe_macros.c b/lib/safe_macros.c index 951e1b064..bb67467b7 100644 --- a/lib/safe_macros.c +++ b/lib/safe_macros.c @@ -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"); +}
Signed-off-by: Wei Gao <wegao@suse.com> --- include/tst_memutils.h | 6 ++++++ include/tst_safe_macros.h | 2 ++ lib/safe_macros.c | 16 ++++++++++++++++ lib/tst_memutils.c | 5 +++++ 4 files changed, 29 insertions(+)