Hi!
> Signed-off-by: Li Wang <liw...@redhat.com>
> ---
> include/safe_file_ops.h | 5 ++-
> lib/safe_file_ops.c | 92
> ++++++++++++++++++++++++++++++++--------
> testcases/kernel/mem/thp/thp04.c | 10 ++---
> testcases/kernel/mem/thp/thp05.c | 10 ++---
> 4 files changed, 88 insertions(+), 29 deletions(-)
>
> diff --git a/include/safe_file_ops.h b/include/safe_file_ops.h
> index 1815984..17d009e 100644
> --- a/include/safe_file_ops.h
> +++ b/include/safe_file_ops.h
> @@ -42,6 +42,8 @@
> /*
> * All-in-one function to scanf value(s) from a file.
> */
> +int file_scanf(const char *file, const char *fmt, va_list va);
This prototype is wrong, the last one should be ... not va_list.
> void safe_file_scanf(const char *file, const int lineno,
> void (*cleanup_fn)(void),
> const char *path, const char *fmt, ...)
> @@ -50,10 +52,11 @@ void safe_file_scanf(const char *file, const int lineno,
> #define SAFE_FILE_SCANF(cleanup_fn, path, fmt, ...) \
> safe_file_scanf(__FILE__, __LINE__, (cleanup_fn), \
> (path), (fmt), ## __VA_ARGS__)
> -
> /*
> * All-in-one function that lets you printf directly into a file.
> */
> +int file_printf(const char *file, const char *fmt, va_list va);
Here as well.
Have you complied the code? It should have produced quite a lot of
warnings.
> void safe_file_printf(const char *file, const int lineno,
> void (*cleanup_fn)(void),
> const char *path, const char *fmt, ...)
> diff --git a/lib/safe_file_ops.c b/lib/safe_file_ops.c
> index 0325ce2..3ce53ff 100644
> --- a/lib/safe_file_ops.c
> +++ b/lib/safe_file_ops.c
> @@ -73,28 +73,54 @@ static int count_scanf_conversions(const char *fmt)
> return cnt;
> }
>
> +int file_vscanf(const char *file, const char *fmt, va_list va)
This function is not part of the public interface, therefore it would be
better to declare it as static.
> +{
> + FILE *f;
> + int ret;
> +
> + f = fopen(file, "r");
> +
> + if (f == NULL) {
> + return -2;
> + }
LKML coding style preffers not to include curly braces around single
line statements.
> + ret = vfscanf(f, fmt, va);
Close the file here, otherwise you leak memory.
> + return ret;
> +}
> +
> +/* Add file_scanf() to replace SAFE_FILE_SCANF() which used in cleanup()*/
Please do not add useless comments like this.
> +int file_scanf(const char *file, const char *fmt, ...)
> +{
> + va_list va;
> + int ret;
> +
> + va_start(va, fmt);
> + ret = file_vscanf(file, fmt, va);
> + va_end(va);
> +
> + return ret;
> +}
> +
> void safe_file_scanf(const char *file, const int lineno,
> void (*cleanup_fn) (void),
> const char *path, const char *fmt, ...)
> {
> va_list va;
> - FILE *f;
> int exp_convs, ret;
>
> - f = fopen(path, "r");
> + exp_convs = count_scanf_conversions(fmt);
>
> - if (f == NULL) {
> + va_start(va, fmt);
> + ret = file_vscanf(path, fmt, va);
> + va_end(va);
> +
> + if (ret == -2) {
> tst_brkm(TBROK | TERRNO, cleanup_fn,
> "Failed to open FILE '%s' for reading at %s:%d",
> path, file, lineno);
> }
>
> - exp_convs = count_scanf_conversions(fmt);
> -
> - va_start(va, fmt);
> - ret = vfscanf(f, fmt, va);
> - va_end(va);
> -
> if (ret == EOF) {
> tst_brkm(TBROK, cleanup_fn,
> "The FILE '%s' ended prematurely at %s:%d",
> @@ -109,32 +135,62 @@ void safe_file_scanf(const char *file, const int lineno,
>
> }
>
> +int file_vprintf(const char *file, const char *fmt, va_list va)
> +{
Should be static function as well.
> + FILE *f;
> +
> + f = fopen(file, "w");
> +
> + if (f == NULL) {
> + return -2;
> + }
> +
> + if (vfprintf(f, fmt, va) < 0) {
> + return -3;
> + }
> +
> + if (fclose(f)) {
> + return -4;
> + }
Useless curly braces as well.
> +}
> +
> +/* Add file_printf() to replace SAFE_FILE_PRINTF() which used in cleanup()*/
Useless comment as well.
> +int file_printf(const char *file, const char *fmt, ...)
> +{
> + va_list va;
> + int ret;
> +
> + va_start(va, fmt);
> + ret = file_vprintf(file, fmt, va);
> + va_end(va);
> +
> + return ret;
> +}
> +
> void safe_file_printf(const char *file, const int lineno,
> void (*cleanup_fn) (void),
> const char *path, const char *fmt, ...)
> {
> va_list va;
> - FILE *f;
> + int ret;
>
> - f = fopen(path, "w");
> + va_start(va, fmt);
> + ret = file_vprintf(path, fmt, va);
> + va_end(va);
>
> - if (f == NULL) {
> + if (ret == -2) {
> tst_brkm(TBROK | TERRNO, cleanup_fn,
> "Failed to open FILE '%s' for writing at %s:%d",
> path, file, lineno);
> }
>
> - va_start(va, fmt);
> -
> - if (vfprintf(f, fmt, va) < 0) {
> + if (ret == -3) {
> tst_brkm(TBROK, cleanup_fn,
> "Failed to print to FILE '%s' at %s:%d",
> path, file, lineno);
> }
>
> - va_end(va);
> -
> - if (fclose(f)) {
> + if (ret == -4) {
> tst_brkm(TBROK | TERRNO, cleanup_fn,
> "Failed to close FILE '%s' at %s:%d",
> path, file, lineno);
> diff --git a/testcases/kernel/mem/thp/thp04.c
> b/testcases/kernel/mem/thp/thp04.c
> index 0b6baeb..db9defc 100644
> --- a/testcases/kernel/mem/thp/thp04.c
> +++ b/testcases/kernel/mem/thp/thp04.c
> @@ -120,10 +120,10 @@ void setup(void)
>
> void cleanup(void)
> {
> - SAFE_FILE_PRINTF(NULL, PATH_KHPD "scan_sleep_millisecs",
> + file_printf(PATH_KHPD "scan_sleep_millisecs",
> "%d", pre_thp_scan_sleep_millisecs);
>
> - SAFE_FILE_PRINTF(NULL, PATH_KHPD "alloc_sleep_millisecs",
> + file_printf(PATH_KHPD "alloc_sleep_millisecs",
> "%d", pre_thp_alloc_sleep_millisecs);
>
> /*
> @@ -132,11 +132,11 @@ void cleanup(void)
> * the three mode: always, madvise, never.
> */
> if (strcmp(pre_thp_enabled, "[always] madvise never") == 0)
> - SAFE_FILE_PRINTF(NULL, PATH_THP "enabled", "always");
> + file_printf(PATH_THP "enabled", "always");
> else if (strcmp(pre_thp_enabled, "always [madvise] never") == 0)
> - SAFE_FILE_PRINTF(NULL, PATH_THP "enabled", "madvise");
> + file_printf(PATH_THP "enabled", "madvise");
> else
> - SAFE_FILE_PRINTF(NULL, PATH_THP "enabled", "never");
> + file_printf(PATH_THP "enabled", "never");
>
> TEST_CLEANUP;
> }
> diff --git a/testcases/kernel/mem/thp/thp05.c
> b/testcases/kernel/mem/thp/thp05.c
> index 8b595ca..b20d9bd 100644
> --- a/testcases/kernel/mem/thp/thp05.c
> +++ b/testcases/kernel/mem/thp/thp05.c
> @@ -129,18 +129,18 @@ void setup(void)
>
> void cleanup(void)
> {
> - SAFE_FILE_PRINTF(NULL, PATH_KHPD "scan_sleep_millisecs",
> + file_printf(PATH_KHPD "scan_sleep_millisecs",
> "%d", pre_thp_scan_sleep_millisecs);
>
> - SAFE_FILE_PRINTF(NULL, PATH_KHPD "alloc_sleep_millisecs",
> + file_printf(PATH_KHPD "alloc_sleep_millisecs",
> "%d", pre_thp_alloc_sleep_millisecs);
>
> if (strcmp(pre_thp_enabled, "[always] madvise never") == 0)
> - SAFE_FILE_PRINTF(NULL, PATH_THP "enabled", "always");
> + file_printf(PATH_THP "enabled", "always");
> else if (strcmp(pre_thp_enabled, "always [madvise] never") == 0)
> - SAFE_FILE_PRINTF(NULL, PATH_THP "enabled", "madvise");
> + file_printf(PATH_THP "enabled", "madvise");
> else
> - SAFE_FILE_PRINTF(NULL, PATH_THP "enabled", "never");
> + file_printf(PATH_THP "enabled", "never");
Looking at the change here, it would be better if file_printf() will
issue tst_resm(TWARN, "xyz has failed") in case that the file_printf()
will fail. Because otherwise it would be close to impossible to track
down the failure. Maybe it would be even better to create FILE_PRINTF()
macro similary to the SAFE_FILE_PRINTF(), what do you think?
--
Cyril Hrubis
chru...@suse.cz
------------------------------------------------------------------------------
Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server
from Actuate! Instantly Supercharge Your Business Reports and Dashboards
with Interactivity, Sharing, Native Excel Exports, App Integration & more
Get technology previously reserved for billion-dollar corporations, FREE
http://pubads.g.doubleclick.net/gampad/clk?id=157005751&iu=/4140/ostg.clktrk
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list
