/vm/hugepage/thp/thp05: cleanup should not use safe macros

PATCH Cyril Hrubis 1 participant 1 message Dec 2, 2014 2 messages not in the archive

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Cyril Hrubis

author

2014-12-02 12:00 UTC #2 lore raw

Hi!

Quoted 13 lines

Signed-off-by Li Wang <[email protected]>

---
 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(-)

include/safe_file_ops.h +0 -0

index 1815984..17d009e 100644

include/safe_file_ops.h line 45

Quoted 4 lines

include/safe_file_ops.h +1 -0

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

include/safe_file_ops.h void safe_file_scanf(const char *file, const int lineno, · line 58

Quoted 12 lines

include/safe_file_ops.h +1 -1

 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.

lib/safe_file_ops.c static int count_scanf_conversions(const char *fmt) · line 76

Quoted 12 lines

include/safe_file_ops.h +0 -0

 void safe_file_printf(const char *file, const int lineno,
                       void (*cleanup_fn)(void),
                       const char *path, const char *fmt, ...)

lib/safe_file_ops.c +1 -0

index 0325ce2..3ce53ff 100644
@@ -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.

lib/safe_file_ops.c static int count_scanf_conversions(const char *fmt) · line 85

Quoted 9 lines

lib/safe_file_ops.c +9 -0

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

lib/safe_file_ops.c static int count_scanf_conversions(const char *fmt) · line 86

Quoted 1 line

lib/safe_file_ops.c +1 -0
+	ret = vfscanf(f, fmt, va);

Close the file here, otherwise you leak memory.

lib/safe_file_ops.c static int count_scanf_conversions(const char *fmt) · line 90

Quoted 4 lines

lib/safe_file_ops.c +4 -0

+	return ret;
+}
+
+/* Add file_scanf() to replace SAFE_FILE_SCANF() which used in cleanup()*/

Please do not add useless comments like this.

Quoted 44 lines

lib/safe_file_ops.c +18 -9

+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",

lib/safe_file_ops.c void safe_file_scanf(const char *file, const int lineno, · line 139

Quoted 5 lines

lib/safe_file_ops.c +2 -0

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

Quoted 5 lines

lib/safe_file_ops.c +5 -0

+	FILE *f;
+
+	f = fopen(file, "w");
+
+	if (f == NULL) {

lib/safe_file_ops.c void safe_file_scanf(const char *file, const int lineno, · line 154

Quoted 10 lines

lib/safe_file_ops.c +10 -0

+		return -2;
+	}
+
+	if (vfprintf(f, fmt, va) < 0) {
+		return -3;
+	}
+
+	if (fclose(f)) {
+		return -4;
+	}

Useless curly braces as well.

lib/safe_file_ops.c void safe_file_scanf(const char *file, const int lineno, · line 157

Quoted 3 lines

lib/safe_file_ops.c +3 -0

+}
+
+/* Add file_printf() to replace SAFE_FILE_PRINTF() which used in cleanup()*/

Useless comment as well.

Quoted 95 lines

lib/safe_file_ops.c +19 -9

+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);

testcases/kernel/mem/thp/thp04.c +5 -5

index 0b6baeb..db9defc 100644
@@ -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;
 }

testcases/kernel/mem/thp/thp05.c +2 -2

index 8b595ca..b20d9bd 100644
@@ -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);

testcases/kernel/mem/thp/thp05.c void setup(void) · line 143

Quoted 10 lines

testcases/kernel/mem/thp/thp05.c +3 -3

 
 	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?

Signature · 13 lines

Cyril Hrubis
[email protected]

------------------------------------------------------------------------------
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
[email protected]
https://lists.sourceforge.net/lists/listinfo/ltp-list