Re: [LTP] [PATCH] /vm/hugepage/thp/thp05: cleanup should not use safe macros

Re: [LTP] [PATCH] /vm/hugepage/thp/thp05: cleanup should not use safe macros

[Cyril Hrubis]

Hi!
Rebased and pushed with a few cosmetic changes (see below), thanks.

> +int file_scanf(const char *file, const int lineno,
> +		     const char *path, const char *fmt, ...)
> +{
> +	va_list va;
> +	FILE *f;
> +	int exp_convs, ret;
> +
> +	f = fopen(path, "r");
> +
> +	if (f == NULL) {
> +		tst_resm(TWARN,
> +			"Failed to close FILE '%s' at %s:%d",
                                   ^
				   open
> +			 path, file, lineno);
> +		return 1;
> +	}
> +

...

> +	if (fclose(f)) {
> +		tst_resm(TWARN,
> +			 "Failed to close FILE '%s' at %s:%d",
> +			 path, file, lineno);
> +		return 1;
> +	}
> +
> +	return 0;
> +
> +err:
> +	if (fclose(f)) {
> +		tst_resm(TWARN,
> +			 "Failed to close FILE '%s' at %s:%d",
> +			 path, file, lineno);
> +		return 1;
                 ^
		 This is not needed because of the return 1 right below.
> +	}
> +	return 1;
> +}

-- 
Cyril Hrubis
chrubis@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=164703151&iu=/4140/ostg.clktrk
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list

Re: [LTP] [PATCH] /vm/hugepage/thp/thp05: cleanup should not use safe macros

[Cyril Hrubis]

Hi!
> 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 <liwang@redhat.com>
> ---
>  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(-)
> 
> diff --git a/include/safe_file_ops.h b/include/safe_file_ops.h
> index 1815984..865575a 100644
> --- a/include/safe_file_ops.h
> +++ b/include/safe_file_ops.h
> @@ -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.

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

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

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

> +	}
> +
> +	exp_convs = count_scanf_conversions(fmt);
> +
> +	va_start(va, fmt);
> +	ret = vfscanf(f, fmt, va);
> +	va_end(va);
> +
> +	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;

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

-- 
Cyril Hrubis
chrubis@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=164703151&iu=/4140/ostg.clktrk
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list

Re: [LTP] [PATCH] /vm/hugepage/thp/thp05: cleanup should not use safe macros

[Cyril Hrubis]

Hi!
> Signed-off-by: Li Wang <liwang@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
chrubis@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

[LTP] [PATCH] /vm/hugepage/thp/thp05: cleanup should not use safe macros

[Li Wang]

NOTE: It's wise NOT to use safe macros in test cleanup(). This is because
      all safe macros call tst_brkm(), which exits the test immediately, making
      the cleanup() exit prematurely. (Actually, this is hacked around in
      the test library at the moment so that the cleanup() will finish, but
      the hack will be removed in the future).

Signed-off-by: Li Wang <liwan@redhat.com>
---
 testcases/kernel/mem/thp/thp05.c | 69 ++++++++++++++++++++++++++++++++--------
 1 file changed, 56 insertions(+), 13 deletions(-)

diff --git a/testcases/kernel/mem/thp/thp05.c b/testcases/kernel/mem/thp/thp05.c
index 8b595ca..92d9610 100644
--- a/testcases/kernel/mem/thp/thp05.c
+++ b/testcases/kernel/mem/thp/thp05.c
@@ -66,6 +66,7 @@ option_t thp_options[] = {
 static int pre_thp_scan_sleep_millisecs;
 static int pre_thp_alloc_sleep_millisecs;
 static char pre_thp_enabled[BUFSIZ];
+static char buf[BUFSIZ], path[BUFSIZ];
 
 int main(int argc, char *argv[])
 {
@@ -129,19 +130,61 @@ void setup(void)
 
 void cleanup(void)
 {
-	SAFE_FILE_PRINTF(NULL, PATH_KHPD "scan_sleep_millisecs",
-			 "%d", pre_thp_scan_sleep_millisecs);
-
-	SAFE_FILE_PRINTF(NULL, 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");
-	else if (strcmp(pre_thp_enabled, "always [madvise] never") == 0)
-		SAFE_FILE_PRINTF(NULL, PATH_THP "enabled", "madvise");
-	else
-		SAFE_FILE_PRINTF(NULL, PATH_THP "enabled", "never");
-
+	int fd;
+
+	/* restore the scan_sleep_millisecs to original value */
+	snprintf(buf, BUFSIZ, "%d", pre_thp_scan_sleep_millisecs);
+	fd = open(PATH_KHPD "scan_sleep_millisecs", O_WRONLY);
+	if (fd == -1)
+		tst_resm(TWARN | TERRNO, "open");
+	tst_resm(TINFO, "restore scan_sleep_millisecs to %d",
+			pre_thp_scan_sleep_millisecs);
+	if (write(fd, buf, strlen(buf)) != strlen(buf))
+		tst_resm(TWARN | TERRNO, "write");
+	close(fd);
+
+	/* restore the alloc_sleep_millisecs to original value */
+	snprintf(buf, BUFSIZ, "%d", pre_thp_alloc_sleep_millisecs);
+	fd = open(PATH_KHPD "alloc_sleep_millisecs", O_WRONLY);
+	if (fd == -1)
+		tst_resm(TWARN | TERRNO, "open");
+	tst_resm(TINFO, "restore alloc_sleep_millisecs to %d",
+			pre_thp_alloc_sleep_millisecs);
+	if (write(fd, buf, strlen(buf)) != strlen(buf))
+		tst_resm(TWARN | TERRNO, "write");
+	close(fd);
+
+	/* restore the transparent_hugepage options to original state */
+	if (strcmp(pre_thp_enabled, "[always] madvise never") == 0){
+		snprintf(path, BUFSIZ, "always");
+		fd = open(PATH_THP "enabled", O_WRONLY);
+		if (fd == -1)
+			tst_resm(TWARN | TERRNO, "open");
+		tst_resm(TINFO, "restore transparent_hugepage to %s", path);
+		if (write(fd, path, strlen(path)) != strlen(path))
+			tst_resm(TWARN | TERRNO, "write");
+		close(fd);
+	}
+	else if (strcmp(pre_thp_enabled, "always [madvise] never") == 0){
+		snprintf(path, BUFSIZ, "madvise");
+		fd = open(PATH_THP "enabled", O_WRONLY);
+		if (fd == -1)
+			tst_resm(TWARN | TERRNO, "open");
+		tst_resm(TINFO, "restore transparent_hugepage to %s", path);
+		if (write(fd, path, strlen(path)) != strlen(path))
+			tst_resm(TWARN | TERRNO, "write");
+		close(fd);
+	}
+	else{
+		snprintf(path, BUFSIZ, "never");
+		fd = open(PATH_THP "enabled", O_WRONLY);
+		if (fd == -1)
+			tst_resm(TWARN | TERRNO, "open");
+		tst_resm(TINFO, "restore transparent_hugepage to %s", path);
+		if (write(fd, path, strlen(path)) != strlen(path))
+			tst_resm(TWARN | TERRNO, "write");
+		close(fd);
+	}
 	TEST_CLEANUP;
 }
 
-- 
1.9.3


------------------------------------------------------------------------------
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list

Re: [LTP] [PATCH] /vm/hugepage/thp/thp05: cleanup should not use safe macros

[Cyril Hrubis]

Hi!
> NOTE: It's wise NOT to use safe macros in test cleanup(). This is because
>       all safe macros call tst_brkm(), which exits the test immediately, making
>       the cleanup() exit prematurely. (Actually, this is hacked around in
>       the test library at the moment so that the cleanup() will finish, but
>       the hack will be removed in the future).
> 
> Signed-off-by: Li Wang <liwan@redhat.com>
> ---
>  testcases/kernel/mem/thp/thp05.c | 69 ++++++++++++++++++++++++++++++++--------
>  1 file changed, 56 insertions(+), 13 deletions(-)
> 
> diff --git a/testcases/kernel/mem/thp/thp05.c b/testcases/kernel/mem/thp/thp05.c
> index 8b595ca..92d9610 100644
> --- a/testcases/kernel/mem/thp/thp05.c
> +++ b/testcases/kernel/mem/thp/thp05.c
> @@ -66,6 +66,7 @@ option_t thp_options[] = {
>  static int pre_thp_scan_sleep_millisecs;
>  static int pre_thp_alloc_sleep_millisecs;
>  static char pre_thp_enabled[BUFSIZ];
> +static char buf[BUFSIZ], path[BUFSIZ];
>  
>  int main(int argc, char *argv[])
>  {
> @@ -129,19 +130,61 @@ void setup(void)
>  
>  void cleanup(void)
>  {
> -	SAFE_FILE_PRINTF(NULL, PATH_KHPD "scan_sleep_millisecs",
> -			 "%d", pre_thp_scan_sleep_millisecs);
> -
> -	SAFE_FILE_PRINTF(NULL, 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");
> -	else if (strcmp(pre_thp_enabled, "always [madvise] never") == 0)
> -		SAFE_FILE_PRINTF(NULL, PATH_THP "enabled", "madvise");
> -	else
> -		SAFE_FILE_PRINTF(NULL, PATH_THP "enabled", "never");
> -
> +	int fd;
> +
> +	/* restore the scan_sleep_millisecs to original value */
> +	snprintf(buf, BUFSIZ, "%d", pre_thp_scan_sleep_millisecs);
> +	fd = open(PATH_KHPD "scan_sleep_millisecs", O_WRONLY);
> +	if (fd == -1)
> +		tst_resm(TWARN | TERRNO, "open");
> +	tst_resm(TINFO, "restore scan_sleep_millisecs to %d",
> +			pre_thp_scan_sleep_millisecs);
> +	if (write(fd, buf, strlen(buf)) != strlen(buf))
> +		tst_resm(TWARN | TERRNO, "write");
> +	close(fd);
> +
> +	/* restore the alloc_sleep_millisecs to original value */
> +	snprintf(buf, BUFSIZ, "%d", pre_thp_alloc_sleep_millisecs);
> +	fd = open(PATH_KHPD "alloc_sleep_millisecs", O_WRONLY);
> +	if (fd == -1)
> +		tst_resm(TWARN | TERRNO, "open");
> +	tst_resm(TINFO, "restore alloc_sleep_millisecs to %d",
> +			pre_thp_alloc_sleep_millisecs);
> +	if (write(fd, buf, strlen(buf)) != strlen(buf))
> +		tst_resm(TWARN | TERRNO, "write");
> +	close(fd);
> +
> +	/* restore the transparent_hugepage options to original state */
> +	if (strcmp(pre_thp_enabled, "[always] madvise never") == 0){
> +		snprintf(path, BUFSIZ, "always");
> +		fd = open(PATH_THP "enabled", O_WRONLY);
> +		if (fd == -1)
> +			tst_resm(TWARN | TERRNO, "open");
> +		tst_resm(TINFO, "restore transparent_hugepage to %s", path);
> +		if (write(fd, path, strlen(path)) != strlen(path))
> +			tst_resm(TWARN | TERRNO, "write");
> +		close(fd);
> +	}
> +	else if (strcmp(pre_thp_enabled, "always [madvise] never") == 0){
> +		snprintf(path, BUFSIZ, "madvise");
> +		fd = open(PATH_THP "enabled", O_WRONLY);
> +		if (fd == -1)
> +			tst_resm(TWARN | TERRNO, "open");
> +		tst_resm(TINFO, "restore transparent_hugepage to %s", path);
> +		if (write(fd, path, strlen(path)) != strlen(path))
> +			tst_resm(TWARN | TERRNO, "write");
> +		close(fd);
> +	}
> +	else{
> +		snprintf(path, BUFSIZ, "never");
> +		fd = open(PATH_THP "enabled", O_WRONLY);
> +		if (fd == -1)
> +			tst_resm(TWARN | TERRNO, "open");
> +		tst_resm(TINFO, "restore transparent_hugepage to %s", path);
> +		if (write(fd, path, strlen(path)) != strlen(path))
> +			tst_resm(TWARN | TERRNO, "write");
> +		close(fd);
> +	}

While generally avoiding the safe macros in cleanup is fine, in this
case you have replaced about ten lines of code with about hundred lines
of copy & pasted code and even the coding style is wrong.

What should be done in this case is to create an equivalent to the
SAFE_FILE_PRINTF() function that is not calling the tst_brkm() and
issues only a warning. Or modify SAFE_FILE_PRINTF() to have additional
parameter that selects if on failure tst_brkm() or tst_resm(TWARN, ...)
is called.

-- 
Cyril Hrubis
chrubis@suse.cz

------------------------------------------------------------------------------
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list

Re: [LTP] [PATCH] /vm/hugepage/thp/thp05: cleanup should not use safe macros

[Cyril Hrubis]

Hi!
> > What should be done in this case is to create an equivalent to the
> > SAFE_FILE_PRINTF() function that is not calling the tst_brkm() and
> 
> 
> well, if create a new safe function/macro to replace use the SAFE_FILE_PRINTF
> but not calling tst_brkm(), that's will be break the safe rule.  
> if create a unsafe function/macro, I dont know where is to place the 
> new function and how to rename. 
> 
> Additional, i found so much testcases use safe macro in there cleanup(), that's 
> really a hard work if maintain all.

The ideal solution would be to create a base functions file_scanf() and
file_printf() that would look like safe_file_scanf() and
safe_file_printf() but without the file, lineno and cleanup parameters.

These would return non-zero return value on failure and then we can base the
safe_file_scanf() on the top of these functions.

I.e. safe_file_scanf() would call file_scanf() and call tst_brkm() if
file_scanf() has reported a failure. Then we can use file_scanf() in test
cleanup. But as these functions use variable number of arguments this needs one
more indirection with functions that takes va_list.

int file_vscanf(const char *file, const char *fmt, va_list va)
{
	// do here what safe_file_scanf() does but return with error
        // instead of tst_brkm()
}

int file_scanf(const char *file, const char *fmt, ...)
{
	va_list va;

	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, ...)
{
	// call file_vscanf() here and switch on it's exit value
        // i.e. one for failed to open file
        //      one for EOF
        //      one for unexpected number of conversion
        //      and exit cleanly on zero
}

-- 
Cyril Hrubis
chrubis@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