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

PATCH Cyril Hrubis 1 participant 1 message Dec 3, 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-03 10:50 UTC #2 lore raw

Hi!

Quoted 30 lines

Hi, after thinking over, I feel there is no need to create a static function file_vscanf() and file_vprintf().
I'd love to create FILE_PRINTF() macro that is not calling the tst_brkm() and issues only a warning. Then we can use FILE_PRINTF() to replace all the safe macros in cleanup().
So, new patch like this:
Signed-off-by Li Wang <[email protected]>
---
 include/safe_file_ops.h                            | 10 ++++
 lib/safe_file_ops.c                                | 59 ++++++++++++++++++++++
 .../kernel/device-drivers/acpi/ltp_acpi_cpufreq.c  |  4 +-
 .../kernel/mem/hugetlb/hugeshmget/hugeshmget03.c   |  2 +-
 testcases/kernel/mem/ksm/ksm01.c                   |  2 +-
 testcases/kernel/mem/ksm/ksm02.c                   |  2 +-
 testcases/kernel/mem/ksm/ksm03.c                   |  2 +-
 testcases/kernel/mem/ksm/ksm04.c                   |  2 +-
 testcases/kernel/mem/ksm/ksm06.c                   |  6 +--
 testcases/kernel/mem/thp/thp04.c                   | 10 ++--
 testcases/kernel/mem/thp/thp05.c                   | 10 ++--
 testcases/kernel/syscalls/fork/fork13.c            |  2 +-
 12 files changed, 90 insertions(+), 21 deletions(-)
include/safe_file_ops.h +0 -0
index 1815984..865575a 100644

include/safe_file_ops.h line 46

Quoted 5 lines

include/safe_file_ops.h +2 -0

@@ -42,6 +42,11 @@
 /*
  * All-in-one function to scanf value(s) from a file.
  */
+void file_scanf(const char *file, const char *fmt, ...);
+

In this case we should return some exit value. Simple 0 == ok, 1 == failed, would suffice. Otherwise we cannot say if the values were read or not.

include/safe_file_ops.h line 48

Quoted 2 lines

include/safe_file_ops.h +2 -0

+#define FILE_SCANF(file, fmt, ...) \
+	file_scanf((file), (fmt), ## __VA_ARGS__)

The whole point of the macro indirection is to include the file and lineno parameter in the tst_ messages. Please do that.

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

Quoted 11 lines

include/safe_file_ops.h +4 -0

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

@@ -54,6 +59,11 @@ void safe_file_scanf(const char *file, const int lineno,
 /*
  * All-in-one function that lets you printf directly into a file.
  */
+void file_printf(const char *file, const char *fmt, ...);
+
+#define FILE_PRINTF(file, fmt, ...) \
+	file_printf((file), (fmt), ## __VA_ARGS__)

It would be better to return exit value here as well.

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..6c25ea5 100644
@@ -73,6 +73,37 @@ static int count_scanf_conversions(const char *fmt)
 	return cnt;
 }
 
+void file_scanf(const char *file, const char *fmt, ...)

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

Quoted 10 lines

lib/safe_file_ops.c +10 -0

+{
+	va_list va;
+	FILE *f;
+	int exp_convs, ret;
+
+	f = fopen(file, "r");
+
+	if (f == NULL) {
+		tst_resm(TWARN,
+			"Failed to open FILE '%s' for reading", file);

You have to do a return here otherwise we will continue the execution till the vfscanf() below and segfault.

Quoted 8 lines

lib/safe_file_ops.c +8 -0

+	}
+
+	exp_convs = count_scanf_conversions(fmt);
+
+	va_start(va, fmt);
+	ret = vfscanf(f, fmt, va);
+	va_end(va);
+

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

Quoted 10 lines

lib/safe_file_ops.c +10 -0

+	if (ret == EOF)
+		tst_resm(TWARN, "The FILE '%s' ended prematurely", file);
+
+	if (ret != exp_convs) {
+		tst_resm(TWARN, "Expected %i conversions got %i FILE '%s'",
+			 exp_convs, ret, file);
+	}
+
+	if (fclose(f))
+		tst_resm(TWARN, "Failed to close FILE '%s'", file);

Missing returns here as well, but do not forget to close the file. I.e. do

	if (ret == EOF) {
		tst_resm(TWARN, ...);
		goto err;
	}

	...

	return 0;
err:
	fclose(f)
	return 1;

Quoted 28 lines

lib/safe_file_ops.c +21 -0

+}
+
 void safe_file_scanf(const char *file, const int lineno,
 		     void (*cleanup_fn) (void),
 		     const char *path, const char *fmt, ...)

@@ -107,6 +138,34 @@ void safe_file_scanf(const char *file, const int lineno,
 			 exp_convs, ret, path, file, lineno);
 	}
 
+	if (fclose(f)) {
+		tst_brkm(TBROK | TERRNO, cleanup_fn,
+			 "Failed to close FILE '%s' at %s:%d",
+			 path, file, lineno);
+	}
+}
+
+void file_printf(const char *file, const char *fmt, ...)
+{
+	va_list va;
+	FILE *f;
+
+	f = fopen(file, "w");
+
+	if (f == NULL) {
+		tst_resm(TWARN,
+			"Failed to open FILE '%s' for writing", file);
+	}
+

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

Quoted 10 lines

lib/safe_file_ops.c +9 -0

+	va_start(va, fmt);
+
+	if (vfprintf(f, fmt, va) < 0)
+		tst_resm(TWARN, "Failed to print to FILE '%s'", file);
+
+	va_end(va);
+
+	if (fclose(f))
+		tst_resm(TWARN, "Failed to close FILE '%s'", file);
 }

Missing return statements here as well.

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=164703151&iu=/4140/ostg.clktrk
_______________________________________________
Ltp-list mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/ltp-list