[LTP] [PATCH 1/2] SAFE_MACROS: Redirect to tst_brk_() early

[LTP] [PATCH 1/2] SAFE_MACROS: Redirect to tst_brk_() early

[Cyril Hrubis]

This is first patch of a patchset that would allow us to use
SAFE_MACROS() in newlib testcases. After the patch we redirect to
tst_brk_() in case of newlib tests directly in the safe_* functions.

This is needed since the tst_brkm_() from the old library is marked as
attribute ((noreturn)) and because of that the return address is not
saved on stack and hence we cannot return from the function. Removing
the atrribute is not an option either since that generates ~1000
"control reaches end of non-void function" warnings.

So this commit adds brkm_redirect.h internal header that defines macros
that calls tst_brkm_()/tst_brk_() depending on if we are running
oldlib/newlib testcase, that are then applied to the safe macros
sources. With that the code never reaches the tst_brkm_() from which we
cannot return.

Signed-off-by: Cyril Hrubis <chrubis@suse.cz>
---
 lib/brkm_redirect.h | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
 lib/safe_file_ops.c | 35 ++++++++++++++++++-----------------
 lib/safe_macros.c   |  1 +
 lib/safe_net.c      |  5 +++--
 lib/safe_stdio.c    |  1 +
 5 files changed, 71 insertions(+), 19 deletions(-)
 create mode 100644 lib/brkm_redirect.h

diff --git a/lib/brkm_redirect.h b/lib/brkm_redirect.h
new file mode 100644
index 0000000..4ef0679
--- /dev/null
+++ b/lib/brkm_redirect.h
@@ -0,0 +1,48 @@
+/*
+ * Copyright (c) 2017 Cyril Hrubis chrubis@suse.cz
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA  02111-1307, USA.
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of version 2 of the GNU General Public License as
+ * published by the Free Software Foundation.
+ */
+
+#ifndef BRKM_REDIRECT_H__
+#define BRKM_REDIRECT_H__
+
+#include "ltp_priv.h"
+
+#ifdef tst_brkm
+# undef tst_brkm
+#endif
+
+#define tst_brkm(flags, cleanup, fmt, ...) do { \
+	if (tst_test) \
+		tst_brk_(__FILE__, __LINE__, flags, fmt, ##__VA_ARGS__); \
+	else \
+                tst_brkm_(__FILE__, __LINE__, flags, cleanup, fmt, ##__VA_ARGS__); \
+	} while (0)
+
+#define tst_brkr(rval, flags, cleanup, fmt, ...) do { \
+		tst_brkm(flags, cleanup, fmt, ##__VA_ARGS__); \
+		return rval; \
+	} while (0)
+
+#define tst_brkv(flags, cleanup, fmt, ...) do { \
+		tst_brkm(flags, cleanup, fmt, ##__VA_ARGS__); \
+		return; \
+	} while (0)
+
+#endif /* BRKM_REDIRECT_H__ */
diff --git a/lib/safe_file_ops.c b/lib/safe_file_ops.c
index 01f64ed..51853b4 100644
--- a/lib/safe_file_ops.c
+++ b/lib/safe_file_ops.c
@@ -33,6 +33,7 @@
 
 #include "test.h"
 #include "safe_file_ops_fn.h"
+#include "brkm_redirect.h"
 
 /*
  * Count number of expected assigned conversions. Any conversion starts with '%'.
@@ -139,7 +140,7 @@ void safe_file_scanf(const char *file, const int lineno,
 	f = fopen(path, "r");
 
 	if (f == NULL) {
-		tst_brkm(TBROK | TERRNO, cleanup_fn,
+		tst_brkv(TBROK | TERRNO, cleanup_fn,
 			 "Failed to open FILE '%s' for reading at %s:%d",
 			 path, file, lineno);
 	}
@@ -151,19 +152,19 @@ void safe_file_scanf(const char *file, const int lineno,
 	va_end(va);
 
 	if (ret == EOF) {
-		tst_brkm(TBROK, cleanup_fn,
+		tst_brkv(TBROK, cleanup_fn,
 			 "The FILE '%s' ended prematurely at %s:%d",
 			 path, file, lineno);
 	}
 
 	if (ret != exp_convs) {
-		tst_brkm(TBROK, cleanup_fn,
+		tst_brkv(TBROK, cleanup_fn,
 			 "Expected %i conversions got %i FILE '%s' at %s:%d",
 			 exp_convs, ret, path, file, lineno);
 	}
 
 	if (fclose(f)) {
-		tst_brkm(TBROK | TERRNO, cleanup_fn,
+		tst_brkv(TBROK | TERRNO, cleanup_fn,
 			 "Failed to close FILE '%s' at %s:%d",
 			 path, file, lineno);
 	}
@@ -186,13 +187,13 @@ int file_lines_scanf(const char *file, const int lineno,
 	va_list ap;
 
 	if (!fmt) {
-		tst_brkm(TBROK, cleanup_fn, "pattern is NULL, %s:%d",
+		tst_brkr(-1, TBROK, cleanup_fn, "pattern is NULL, %s:%d",
 			file, lineno);
 	}
 
 	fp = fopen(path, "r");
 	if (fp == NULL) {
-		tst_brkm(TBROK | TERRNO, cleanup_fn,
+		tst_brkr(-1, TBROK | TERRNO, cleanup_fn,
 			"Failed to open FILE '%s' for reading at %s:%d",
 			path, file, lineno);
 	}
@@ -210,7 +211,7 @@ int file_lines_scanf(const char *file, const int lineno,
 	fclose(fp);
 
 	if (strict && ret != arg_count)
-		tst_brkm(TBROK, cleanup_fn, "Expected %i conversions got %i"
+		tst_brkr(-1, TBROK, cleanup_fn, "Expected %i conversions got %i"
 			" at %s:%d", arg_count, ret, file, lineno);
 
 	return !(ret == arg_count);
@@ -270,7 +271,7 @@ void safe_file_printf(const char *file, const int lineno,
 	f = fopen(path, "w");
 
 	if (f == NULL) {
-		tst_brkm(TBROK | TERRNO, cleanup_fn,
+		tst_brkv(TBROK | TERRNO, cleanup_fn,
 			 "Failed to open FILE '%s' for writing at %s:%d",
 			 path, file, lineno);
 	}
@@ -278,7 +279,7 @@ void safe_file_printf(const char *file, const int lineno,
 	va_start(va, fmt);
 
 	if (vfprintf(f, fmt, va) < 0) {
-		tst_brkm(TBROK, cleanup_fn,
+		tst_brkv(TBROK, cleanup_fn,
 			 "Failed to print to FILE '%s' at %s:%d",
 			 path, file, lineno);
 	}
@@ -286,7 +287,7 @@ void safe_file_printf(const char *file, const int lineno,
 	va_end(va);
 
 	if (fclose(f)) {
-		tst_brkm(TBROK | TERRNO, cleanup_fn,
+		tst_brkv(TBROK | TERRNO, cleanup_fn,
 			 "Failed to close FILE '%s' at %s:%d",
 			 path, file, lineno);
 	}
@@ -305,7 +306,7 @@ void safe_cp(const char *file, const int lineno,
 	ret = system(buf);
 
 	if (ret) {
-		tst_brkm(TBROK, cleanup_fn,
+		tst_brkv(TBROK, cleanup_fn,
 			 "Failed to copy '%s' to '%s' at %s:%d",
 			 src, dst, file, lineno);
 	}
@@ -343,20 +344,20 @@ void safe_touch(const char *file, const int lineno,
 
 	ret = open(pathname, O_CREAT | O_WRONLY, defmode);
 	if (ret == -1)
-		tst_brkm(TBROK | TERRNO, cleanup_fn,
+		tst_brkv(TBROK | TERRNO, cleanup_fn,
 			"Failed to open file '%s' at %s:%d",
 			pathname, file, lineno);
 
 	ret = close(ret);
 	if (ret == -1)
-		tst_brkm(TBROK | TERRNO, cleanup_fn,
+		tst_brkv(TBROK | TERRNO, cleanup_fn,
 			"Failed to close file '%s' at %s:%d",
 			pathname, file, lineno);
 
 	if (mode != 0) {
 		ret = chmod(pathname, mode);
 		if (ret == -1)
-			tst_brkm(TBROK | TERRNO, cleanup_fn,
+			tst_brkv(TBROK | TERRNO, cleanup_fn,
 				"Failed to chmod file '%s' at %s:%d",
 				pathname, file, lineno);
 	}
@@ -373,13 +374,13 @@ void safe_touch(const char *file, const int lineno,
 
 		ret = stat(pathname, &sb);
 		if (ret == -1)
-			tst_brkm(TBROK | TERRNO, cleanup_fn,
+			tst_brkv(TBROK | TERRNO, cleanup_fn,
 				"Failed to stat file '%s' at %s:%d",
 				pathname, file, lineno);
 
 		ret = gettimeofday(cotimes, NULL);
 		if (ret == -1)
-			tst_brkm(TBROK | TERRNO, cleanup_fn,
+			tst_brkv(TBROK | TERRNO, cleanup_fn,
 				"Failed to gettimeofday() at %s:%d",
 				file, lineno);
 		cotimes[1] = cotimes[0];
@@ -393,7 +394,7 @@ void safe_touch(const char *file, const int lineno,
 	}
 #endif
 	if (ret == -1) {
-		tst_brkm(TBROK | TERRNO, cleanup_fn,
+		tst_brkv(TBROK | TERRNO, cleanup_fn,
 			"Failed to update the access/modification time on file"
 			" '%s'@%s:%d", pathname, file, lineno);
 	}
diff --git a/lib/safe_macros.c b/lib/safe_macros.c
index d696a0b..efa1897 100644
--- a/lib/safe_macros.c
+++ b/lib/safe_macros.c
@@ -17,6 +17,7 @@
 #include <malloc.h>
 #include "test.h"
 #include "safe_macros.h"
+#include "brkm_redirect.h"
 
 char *safe_basename(const char *file, const int lineno,
 		    void (*cleanup_fn) (void), char *path)
diff --git a/lib/safe_net.c b/lib/safe_net.c
index cae77b5..5ba6b85 100644
--- a/lib/safe_net.c
+++ b/lib/safe_net.c
@@ -19,6 +19,7 @@
 #include <errno.h>
 #include "test.h"
 #include "safe_net_fn.h"
+#include "brkm_redirect.h"
 
 char *tst_sock_addr(const struct sockaddr *sa, socklen_t salen, char *res,
 		    size_t len)
@@ -109,7 +110,7 @@ int safe_bind(const char *file, const int lineno, void (cleanup_fn)(void),
 			return 0;
 
 		if (errno != EADDRINUSE) {
-			tst_brkm(TBROK | TERRNO, cleanup_fn,
+			tst_brkr(-1, TBROK | TERRNO, cleanup_fn,
 				 "%s:%d: bind(%d, %s, %d) failed", file, lineno,
 				 socket, tst_sock_addr(address, address_len,
 						       buf, sizeof(buf)),
@@ -124,7 +125,7 @@ int safe_bind(const char *file, const int lineno, void (cleanup_fn)(void),
 		sleep(1);
 	}
 
-	tst_brkm(TBROK | TERRNO, cleanup_fn,
+	tst_brkr(-1, TBROK | TERRNO, cleanup_fn,
 		 "%s:%d: Failed to bind(%d, %s, %d) after 120 retries", file,
 		 lineno, socket,
 		 tst_sock_addr(address, address_len, buf, sizeof(buf)),
diff --git a/lib/safe_stdio.c b/lib/safe_stdio.c
index 966a039..ef78114 100644
--- a/lib/safe_stdio.c
+++ b/lib/safe_stdio.c
@@ -22,6 +22,7 @@
 #include <errno.h>
 #include "test.h"
 #include "safe_stdio_fn.h"
+#include "brkm_redirect.h"
 
 FILE *safe_fopen(const char *file, const int lineno, void (cleanup_fn)(void),
                  const char *path, const char *mode)
-- 
2.10.2

[LTP] [PATCH 2/2] newlib: Allow SAFE_MACROS to be called from cleanup

[Cyril Hrubis]

Which is done by:

* Dropping attribute ((noreturn)) from all tst_brk_() definitions so
  that the function could actually return in case it's called from
  cleanup.

* Adding brk and res handlers to tst_test.c so that we can temporarily
  replace the tst_brk_() function with function that maps TBROK to TWARN
  and calls tst_vres_().

+ testcase

Signed-off-by: Cyril Hrubis <chrubis@suse.cz>
---
 include/tst_test.h          |  3 +--
 lib/ltp_priv.h              |  2 +-
 lib/newlib_tests/.gitignore |  1 +
 lib/newlib_tests/test14.c   | 44 ++++++++++++++++++++++++++++++++++++++++++++
 lib/tst_test.c              | 28 +++++++++++++++++++++++++---
 5 files changed, 72 insertions(+), 6 deletions(-)
 create mode 100644 lib/newlib_tests/test14.c

diff --git a/include/tst_test.h b/include/tst_test.h
index 7dc371c..a8e01fc 100644
--- a/include/tst_test.h
+++ b/include/tst_test.h
@@ -58,8 +58,7 @@ void tst_resm_hexd_(const char *file, const int lineno, int ttype,
  */
 void tst_brk_(const char *file, const int lineno, int ttype,
               const char *fmt, ...)
-              __attribute__ ((format (printf, 4, 5)))
-              __attribute__ ((noreturn));
+              __attribute__ ((format (printf, 4, 5)));
 
 #define tst_brk(ttype, arg_fmt, ...) \
 	tst_brk_(__FILE__, __LINE__, (ttype), (arg_fmt), ##__VA_ARGS__)
diff --git a/lib/ltp_priv.h b/lib/ltp_priv.h
index de45e4a..e678a28 100644
--- a/lib/ltp_priv.h
+++ b/lib/ltp_priv.h
@@ -59,7 +59,7 @@ void tst_vbrk_(const char *file, const int lineno, int ttype,
                const char *fmt, va_list va) __attribute__((noreturn));
 
 void tst_brk_(const char *file, const int lineno, int ttype,
-              const char *msg, ...) __attribute__((noreturn));
+              const char *msg, ...);
 
 void tst_vres_(const char *file, const int lineno, int ttype,
                const char *fmt, va_list va);
diff --git a/lib/newlib_tests/.gitignore b/lib/newlib_tests/.gitignore
index bc2409c..8e12007 100644
--- a/lib/newlib_tests/.gitignore
+++ b/lib/newlib_tests/.gitignore
@@ -11,6 +11,7 @@ test10
 test11
 test12
 test13
+test14
 tst_device
 tst_safe_fileops
 tst_res_hexd
diff --git a/lib/newlib_tests/test14.c b/lib/newlib_tests/test14.c
new file mode 100644
index 0000000..501bd3a
--- /dev/null
+++ b/lib/newlib_tests/test14.c
@@ -0,0 +1,44 @@
+/*
+ * Copyright (c) 2016 Cyril Hrubis <chrubis@suse.cz>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it would be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write the Free Software Foundation,
+ * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ */
+
+#include "tst_test.h"
+#include "tst_safe_stdio.h"
+#include "tst_safe_net.h"
+
+static void cleanup(void)
+{
+	int i;
+
+	tst_brk(TBROK, "TBROK in cleanup");
+	SAFE_OPEN("foo", O_RDWR);
+	SAFE_FILE_SCANF("foo", "%i", &i);
+	SAFE_FOPEN("foo", "r");
+	SAFE_SOCKET(AF_UNIX, SOCK_STREAM, -1);
+	tst_res(TINFO, "Test still here");
+}
+
+static void do_test(void)
+{
+	tst_res(TPASS, "Passed");
+}
+
+static struct tst_test test = {
+	.tid = "test14",
+	.test_all = do_test,
+	.cleanup = cleanup,
+};
diff --git a/lib/tst_test.c b/lib/tst_test.c
index e78b412..8d528a7 100644
--- a/lib/tst_test.c
+++ b/lib/tst_test.c
@@ -241,12 +241,34 @@ void tst_vres_(const char *file, const int lineno, int ttype,
 }
 
 void tst_vbrk_(const char *file, const int lineno, int ttype,
-               const char *fmt, va_list va) __attribute__((noreturn));
+               const char *fmt, va_list va);
+
+static void (*tst_res_handler)(const char *file, const int lineno, int ttype,
+			       const char *fmt, va_list va) = tst_vres_;
+
+static void (*tst_brk_handler)(const char *file, const int lineno, int ttype,
+			       const char *fmt, va_list va) = tst_vbrk_;
+
+static void tst_cvres(const char *file, const int lineno, int ttype,
+		      const char *fmt, va_list va)
+{
+	if (TTYPE_RESULT(ttype) == TBROK) {
+		ttype &= ~TTYPE_MASK;
+		ttype |= TWARN;
+	}
+
+	print_result(file, lineno, ttype, fmt, va);
+	update_results(file, lineno, TTYPE_RESULT(ttype));
+}
 
 static void do_test_cleanup(void)
 {
+	tst_brk_handler = tst_cvres;
+
 	if (tst_test->cleanup)
 		tst_test->cleanup();
+
+	tst_brk_handler = tst_vbrk_;
 }
 
 void tst_vbrk_(const char *file, const int lineno, int ttype,
@@ -269,7 +291,7 @@ void tst_res_(const char *file, const int lineno, int ttype,
 	va_list va;
 
 	va_start(va, fmt);
-	tst_vres_(file, lineno, ttype, fmt, va);
+	tst_res_handler(file, lineno, ttype, fmt, va);
 	va_end(va);
 }
 
@@ -279,7 +301,7 @@ void tst_brk_(const char *file, const int lineno, int ttype,
 	va_list va;
 
 	va_start(va, fmt);
-	tst_vbrk_(file, lineno, ttype, fmt, va);
+	tst_brk_handler(file, lineno, ttype, fmt, va);
 	va_end(va);
 }
 
-- 
2.10.2

[LTP] [PATCH 1/2] SAFE_MACROS: Redirect to tst_brk_() early

[Jan Stancek]

----- Original Message -----
> From: "Cyril Hrubis" <chrubis@suse.cz>
> To: ltp@lists.linux.it
> Sent: Thursday, 9 February, 2017 3:44:44 PM
> Subject: [LTP] [PATCH 1/2] SAFE_MACROS: Redirect to tst_brk_() early

> +#ifndef BRKM_REDIRECT_H__
> +#define BRKM_REDIRECT_H__
> +
> +#include "ltp_priv.h"
> +
> +#ifdef tst_brkm
> +# undef tst_brkm
> +#endif
> +
> +#define tst_brkm(flags, cleanup, fmt, ...) do { \
> +	if (tst_test) \
> +		tst_brk_(__FILE__, __LINE__, flags, fmt, ##__VA_ARGS__); \
> +	else \
> +                tst_brkm_(__FILE__, __LINE__, flags, cleanup, fmt,
> ##__VA_ARGS__); \
> +	} while (0)

Stil trying to wrap my head around all this, but after first look,..
How does this related to "tst_brkm_" from lib/tst_res.c?
Can it be simplified? It doesn't look like we can reach newlib branch
now, because tst_test condition is now in tst_brkm macro.
Did we loose the check for "Non-NULL cleanup in newlib"?

> +
> +#define tst_brkr(rval, flags, cleanup, fmt, ...) do { \
> +		tst_brkm(flags, cleanup, fmt, ##__VA_ARGS__); \
> +		return rval; \
> +	} while (0)
> +
> +#define tst_brkv(flags, cleanup, fmt, ...) do { \
> +		tst_brkm(flags, cleanup, fmt, ##__VA_ARGS__); \
> +		return; \
> +	} while (0)

All variants of tst_brk_ are more and more tricky to remember.
Would it be too much to ask people "when writing new safe function,
take into account that tst_brkm can return" and leave it up to
them how they do it. Maybe they'll see goto more fitting.

Example:
// stuff in your safe function
if (bad) {
    tst_brkm();
    return;
}
// more stuff in your safe function

> +
> +#endif /* BRKM_REDIRECT_H__ */
> diff --git a/lib/safe_file_ops.c b/lib/safe_file_ops.c
> index 01f64ed..51853b4 100644

[LTP] [PATCH 1/2] SAFE_MACROS: Redirect to tst_brk_() early

[Cyril Hrubis]

Hi!
> > +#ifndef BRKM_REDIRECT_H__
> > +#define BRKM_REDIRECT_H__
> > +
> > +#include "ltp_priv.h"
> > +
> > +#ifdef tst_brkm
> > +# undef tst_brkm
> > +#endif
> > +
> > +#define tst_brkm(flags, cleanup, fmt, ...) do { \
> > +	if (tst_test) \
> > +		tst_brk_(__FILE__, __LINE__, flags, fmt, ##__VA_ARGS__); \
> > +	else \
> > +                tst_brkm_(__FILE__, __LINE__, flags, cleanup, fmt,
> > ##__VA_ARGS__); \
> > +	} while (0)
> 
> Stil trying to wrap my head around all this, but after first look,..
> How does this related to "tst_brkm_" from lib/tst_res.c?
> Can it be simplified? It doesn't look like we can reach newlib branch
> now, because tst_test condition is now in tst_brkm macro.

Well there more library code shared between oldlib and newlib than
SAFE_MACROS. So we can still reach tst_brkm_() from newlib, for instance
tst_device, tst_brkm, ... Hence the redirection has to stay.

Only change after this patch is that it cannot be reached from
SAFE_MACROS().

> Did we loose the check for "Non-NULL cleanup in newlib"?

Not really, since there is no way SAFE_MACRO would pass non-NULL cleanup
callback, since that is hardcoded in the tst_safe_macros.h header.

> > +#define tst_brkr(rval, flags, cleanup, fmt, ...) do { \
> > +		tst_brkm(flags, cleanup, fmt, ##__VA_ARGS__); \
> > +		return rval; \
> > +	} while (0)
> > +
> > +#define tst_brkv(flags, cleanup, fmt, ...) do { \
> > +		tst_brkm(flags, cleanup, fmt, ##__VA_ARGS__); \
> > +		return; \
> > +	} while (0)
> 
> All variants of tst_brk_ are more and more tricky to remember.
> Would it be too much to ask people "when writing new safe function,
> take into account that tst_brkm can return" and leave it up to
> them how they do it. Maybe they'll see goto more fitting.

I wanted to keep the changes minimal, but yes, we can as well
do explicit return instead of tst_brkr() and tst_brkv().

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH 1/2] SAFE_MACROS: Redirect to tst_brk_() early

[Jan Stancek]

----- Original Message -----
> From: "Cyril Hrubis" <chrubis@suse.cz>
> To: "Jan Stancek" <jstancek@redhat.com>
> Cc: ltp@lists.linux.it
> Sent: Thursday, 9 February, 2017 5:11:20 PM
> Subject: Re: [LTP] [PATCH 1/2] SAFE_MACROS: Redirect to tst_brk_() early
> 
> Hi!
> > > +#ifndef BRKM_REDIRECT_H__
> > > +#define BRKM_REDIRECT_H__
> > > +
> > > +#include "ltp_priv.h"
> > > +
> > > +#ifdef tst_brkm
> > > +# undef tst_brkm
> > > +#endif
> > > +
> > > +#define tst_brkm(flags, cleanup, fmt, ...) do { \
> > > +	if (tst_test) \
> > > +		tst_brk_(__FILE__, __LINE__, flags, fmt, ##__VA_ARGS__); \
> > > +	else \
> > > +                tst_brkm_(__FILE__, __LINE__, flags, cleanup, fmt,
> > > ##__VA_ARGS__); \
> > > +	} while (0)
> > 
> > Stil trying to wrap my head around all this, but after first look,..
> > How does this related to "tst_brkm_" from lib/tst_res.c?
> > Can it be simplified? It doesn't look like we can reach newlib branch
> > now, because tst_test condition is now in tst_brkm macro.
> 
> Well there more library code shared between oldlib and newlib than
> SAFE_MACROS. So we can still reach tst_brkm_() from newlib, for instance
> tst_device, tst_brkm, ... Hence the redirection has to stay.

Can't we use macro above directly in include/old.test.h and avoid all
redefinitions later? We would move some common stuff from ltp_priv.h
to include/ header, like forward declaration of tst_test and tst_brk_,
but you get the idea: no macro redefinitions would be needed.

[LTP] [PATCH 1/2] SAFE_MACROS: Redirect to tst_brk_() early

[Jan Stancek]

----- Original Message -----
> From: "Jan Stancek" <jstancek@redhat.com>
> To: "Cyril Hrubis" <chrubis@suse.cz>
> Cc: ltp@lists.linux.it
> Sent: Friday, 10 February, 2017 9:27:07 AM
> Subject: Re: [LTP] [PATCH 1/2] SAFE_MACROS: Redirect to tst_brk_() early
> 
> 
> 
> ----- Original Message -----
> > From: "Cyril Hrubis" <chrubis@suse.cz>
> > To: "Jan Stancek" <jstancek@redhat.com>
> > Cc: ltp@lists.linux.it
> > Sent: Thursday, 9 February, 2017 5:11:20 PM
> > Subject: Re: [LTP] [PATCH 1/2] SAFE_MACROS: Redirect to tst_brk_() early
> > 
> > Hi!
> > > > +#ifndef BRKM_REDIRECT_H__
> > > > +#define BRKM_REDIRECT_H__
> > > > +
> > > > +#include "ltp_priv.h"
> > > > +
> > > > +#ifdef tst_brkm
> > > > +# undef tst_brkm
> > > > +#endif
> > > > +
> > > > +#define tst_brkm(flags, cleanup, fmt, ...) do { \
> > > > +	if (tst_test) \
> > > > +		tst_brk_(__FILE__, __LINE__, flags, fmt, ##__VA_ARGS__); \
> > > > +	else \
> > > > +                tst_brkm_(__FILE__, __LINE__, flags, cleanup, fmt,
> > > > ##__VA_ARGS__); \
> > > > +	} while (0)
> > > 
> > > Stil trying to wrap my head around all this, but after first look,..
> > > How does this related to "tst_brkm_" from lib/tst_res.c?
> > > Can it be simplified? It doesn't look like we can reach newlib branch
> > > now, because tst_test condition is now in tst_brkm macro.
> > 
> > Well there more library code shared between oldlib and newlib than
> > SAFE_MACROS. So we can still reach tst_brkm_() from newlib, for instance
> > tst_device, tst_brkm, ... Hence the redirection has to stay.
> 
> Can't we use macro above directly in include/old.test.h and avoid all
> redefinitions later? We would move some common stuff from ltp_priv.h
> to include/ header, like forward declaration of tst_test and tst_brk_,
> but you get the idea: no macro redefinitions would be needed.

.. which brings back "control reaches end of non-void function" warnings :-/.

[LTP] [PATCH 1/2] SAFE_MACROS: Redirect to tst_brk_() early

[Jan Stancek]

SNIP

How about we provide different version of tst_brkm for library and different
for tests? For library we obviously want to allow calls from both oldlib and
newlib code, and redirect as needed. But for tests (users of library) we
would allow only oldlib tests to call tst_brkm.

So for tests tst_brkm_ stays the same - NORETURN, for library code, we
use either tst_brk_ or tst_brkm_, which means we'll need to fix any
potential warnings only in library.

This also forbids usage of tst_brkm from newlib tests, previously
it was OK if you didn't provide cleanup, now it would reject all
calls from newlib tests.

$ grep -l tst_brkm -r include/
include/old/safe_macros.h
include/old/test.h
include/tst_fs.h -> only comments
include/tst_process_state.h -> only comments

(compile-tested only)

diff --git a/include/old/safe_macros.h b/include/old/safe_macros.h
index 25001e806c12..96fb0c21876e 100644
--- a/include/old/safe_macros.h
+++ b/include/old/safe_macros.h
@@ -331,10 +331,10 @@ static inline int safe_setrlimit(const char *file, const int lineno,

 #define SAFE_IOCTL(cleanup_fn, fd, request, ...)             \
        ({int ret = ioctl(fd, request, __VA_ARGS__);         \
-         ret < 0 ?                                          \
-          tst_brkm(TBROK | TERRNO, cleanup_fn,              \
-                   "ioctl(%i,%s,...) failed", fd, #request) \
-        : ret;})
+         if (ret < 0)                                       \
+             tst_brkm(TBROK | TERRNO, cleanup_fn,           \
+                   "ioctl(%i,%s,...) failed", fd, #request); \
+         ret;})

 #endif /* __SAFE_MACROS_H__ */
 #endif /* __TEST_H__ */
diff --git a/include/old/test.h b/include/old/test.h
index d49256073741..768c32d41609 100644
--- a/include/old/test.h
+++ b/include/old/test.h
@@ -153,9 +153,20 @@ void tst_resm_hexd_(const char *file, const int lineno, int ttype,
 void tst_brkm_(const char *file, const int lineno, int ttype,
        void (*func)(void), const char *arg_fmt, ...)
        __attribute__ ((format (printf, 5, 6))) LTP_ATTRIBUTE_NORETURN;
-#define tst_brkm(ttype, func, arg_fmt, ...) \
-       tst_brkm_(__FILE__, __LINE__, (ttype), (func), \
-                 (arg_fmt), ##__VA_ARGS__)
+
+#ifdef LTPLIB
+#include "ltp_priv.h"
+# define tst_brkm(flags, cleanup, fmt, ...) do { \
+               if (tst_test) \
+                       tst_brk_(__FILE__, __LINE__, flags, fmt, ##__VA_ARGS__); \
+               else \
+                       tst_brkm_(__FILE__, __LINE__, flags, cleanup, fmt, ##__VA_ARGS__); \
+               } while (0)
+#else
+# define tst_brkm(flags, cleanup, fmt, ...) do { \
+                       tst_brkm_(__FILE__, __LINE__, flags, cleanup, fmt, ##__VA_ARGS__); \
+               } while (0)
+#endif

 void tst_require_root(void);
 void tst_exit(void) LTP_ATTRIBUTE_NORETURN;
diff --git a/lib/Makefile b/lib/Makefile
index 663455ee31ad..6b3f6e1917fb 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -24,7 +24,7 @@ top_srcdir            ?= ..

 include $(top_srcdir)/include/mk/env_pre.mk

-CFLAGS                 += -I.
+CFLAGS                 += -I. -DLTPLIB

 ifneq ($(ANDROID),1)
 FILTER_OUT_DIRS                += android_libpthread android_librt
diff --git a/lib/tst_res.c b/lib/tst_res.c
index 96f0c1f1dc07..2b0ffa706cb2 100644
--- a/lib/tst_res.c
+++ b/lib/tst_res.c
@@ -566,16 +566,10 @@ void tst_brkm_(const char *file, const int lineno, int ttype,

        EXPAND_VAR_ARGS(tmesg, arg_fmt, USERMESG);

-       if (tst_test) {
-               if (func) {
-                       tst_brk_(file, lineno, TBROK,
-                                "Non-NULL cleanup in newlib!");
-               }
-
-               tst_brk_(file, lineno, ttype, "%s", tmesg);
-       } else {
+       if (tst_test)
+               tst_brk__(file, lineno, TBROK, NULL, "newlib test calls tst_brkm()");
+       else
                tst_brk__(file, lineno, ttype, func, "%s", tmesg);
-       }

        /* Shouldn't be reached, but fixes build time warnings about noreturn. */
        abort();

[LTP] [PATCH 1/2] SAFE_MACROS: Redirect to tst_brk_() early

[Cyril Hrubis]

Hi!
> How about we provide different version of tst_brkm for library and different
> for tests? For library we obviously want to allow calls from both oldlib and
> newlib code, and redirect as needed. But for tests (users of library) we
> would allow only oldlib tests to call tst_brkm.
> 
> So for tests tst_brkm_ stays the same - NORETURN, for library code, we
> use either tst_brk_ or tst_brkm_, which means we'll need to fix any
> potential warnings only in library.
> 
> This also forbids usage of tst_brkm from newlib tests, previously
> it was OK if you didn't provide cleanup, now it would reject all
> calls from newlib tests.

This sounds good. The only problem we could hit is some common code
outside the lib/ directory used in both newlib and oldlib tests. Which
was the original reason I put the redirection directly into the
tst_brkm_() code...

> $ grep -l tst_brkm -r include/
> include/old/safe_macros.h
> include/old/test.h
> include/tst_fs.h -> only comments
> include/tst_process_state.h -> only comments
> 
> (compile-tested only)
> 
> diff --git a/include/old/safe_macros.h b/include/old/safe_macros.h
> index 25001e806c12..96fb0c21876e 100644
> --- a/include/old/safe_macros.h
> +++ b/include/old/safe_macros.h
> @@ -331,10 +331,10 @@ static inline int safe_setrlimit(const char *file, const int lineno,
> 
>  #define SAFE_IOCTL(cleanup_fn, fd, request, ...)             \
>         ({int ret = ioctl(fd, request, __VA_ARGS__);         \
> -         ret < 0 ?                                          \
> -          tst_brkm(TBROK | TERRNO, cleanup_fn,              \
> -                   "ioctl(%i,%s,...) failed", fd, #request) \
> -        : ret;})
> +         if (ret < 0)                                       \
> +             tst_brkm(TBROK | TERRNO, cleanup_fn,           \
> +                   "ioctl(%i,%s,...) failed", fd, #request); \
> +         ret;})
> 
>  #endif /* __SAFE_MACROS_H__ */
>  #endif /* __TEST_H__ */
> diff --git a/include/old/test.h b/include/old/test.h
> index d49256073741..768c32d41609 100644
> --- a/include/old/test.h
> +++ b/include/old/test.h
> @@ -153,9 +153,20 @@ void tst_resm_hexd_(const char *file, const int lineno, int ttype,
>  void tst_brkm_(const char *file, const int lineno, int ttype,
>         void (*func)(void), const char *arg_fmt, ...)
>         __attribute__ ((format (printf, 5, 6))) LTP_ATTRIBUTE_NORETURN;
> -#define tst_brkm(ttype, func, arg_fmt, ...) \
> -       tst_brkm_(__FILE__, __LINE__, (ttype), (func), \
> -                 (arg_fmt), ##__VA_ARGS__)
> +
> +#ifdef LTPLIB
> +#include "ltp_priv.h"
> +# define tst_brkm(flags, cleanup, fmt, ...) do { \
> +               if (tst_test) \
> +                       tst_brk_(__FILE__, __LINE__, flags, fmt, ##__VA_ARGS__); \
> +               else \
> +                       tst_brkm_(__FILE__, __LINE__, flags, cleanup, fmt, ##__VA_ARGS__); \
> +               } while (0)
> +#else
> +# define tst_brkm(flags, cleanup, fmt, ...) do { \
> +                       tst_brkm_(__FILE__, __LINE__, flags, cleanup, fmt, ##__VA_ARGS__); \
> +               } while (0)

Do we actually need the do { } while (0) here?

> +#endif
> 
>  void tst_require_root(void);
>  void tst_exit(void) LTP_ATTRIBUTE_NORETURN;
> diff --git a/lib/Makefile b/lib/Makefile
> index 663455ee31ad..6b3f6e1917fb 100644
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -24,7 +24,7 @@ top_srcdir            ?= ..
> 
>  include $(top_srcdir)/include/mk/env_pre.mk
> 
> -CFLAGS                 += -I.
> +CFLAGS                 += -I. -DLTPLIB
> 
>  ifneq ($(ANDROID),1)
>  FILTER_OUT_DIRS                += android_libpthread android_librt
> diff --git a/lib/tst_res.c b/lib/tst_res.c
> index 96f0c1f1dc07..2b0ffa706cb2 100644
> --- a/lib/tst_res.c
> +++ b/lib/tst_res.c
> @@ -566,16 +566,10 @@ void tst_brkm_(const char *file, const int lineno, int ttype,
> 
>         EXPAND_VAR_ARGS(tmesg, arg_fmt, USERMESG);
> 
> -       if (tst_test) {
> -               if (func) {
> -                       tst_brk_(file, lineno, TBROK,
> -                                "Non-NULL cleanup in newlib!");
> -               }
> -
> -               tst_brk_(file, lineno, ttype, "%s", tmesg);
> -       } else {
> +       if (tst_test)
> +               tst_brk__(file, lineno, TBROK, NULL, "newlib test calls tst_brkm()");
> +       else
>                 tst_brk__(file, lineno, ttype, func, "%s", tmesg);
> -       }
> 
>         /* Shouldn't be reached, but fixes build time warnings about noreturn. */
>         abort();

Otherwise this looks good.

We would also need to sprinkle the library code with returns as a part
of the patch if I'm not mistaken.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH 1/2] SAFE_MACROS: Redirect to tst_brk_() early

[Jan Stancek]

----- Original Message -----
> From: "Cyril Hrubis" <chrubis@suse.cz>
> To: "Jan Stancek" <jstancek@redhat.com>
> Cc: ltp@lists.linux.it
> Sent: Monday, 13 February, 2017 9:09:29 AM
> Subject: Re: [LTP] [PATCH 1/2] SAFE_MACROS: Redirect to tst_brk_() early
> 
> Hi!
> > How about we provide different version of tst_brkm for library and
> > different
> > for tests? For library we obviously want to allow calls from both oldlib
> > and
> > newlib code, and redirect as needed. But for tests (users of library) we
> > would allow only oldlib tests to call tst_brkm.
> > 
> > So for tests tst_brkm_ stays the same - NORETURN, for library code, we
> > use either tst_brk_ or tst_brkm_, which means we'll need to fix any
> > potential warnings only in library.
> > 
> > This also forbids usage of tst_brkm from newlib tests, previously
> > it was OK if you didn't provide cleanup, now it would reject all
> > calls from newlib tests.
> 
> This sounds good. The only problem we could hit is some common code
> outside the lib/ directory used in both newlib and oldlib tests. Which
> was the original reason I put the redirection directly into the
> tst_brkm_() code...

Good point, I guess we'd have to do the same as for ltp/lib, that
is add -DLTPLIB and deal with warnings.

---

Or we don't make tst_brkm_ so strict, we keep it as it is,
but we find way to prevent newlib users only direct usage
of tst_brkm() macro:

diff --git a/include/old/safe_macros.h b/include/old/safe_macros.h
index 25001e806c12..96fb0c21876e 100644
--- a/include/old/safe_macros.h
+++ b/include/old/safe_macros.h
@@ -331,10 +331,10 @@ static inline int safe_setrlimit(const char *file, const int lineno,
 
 #define SAFE_IOCTL(cleanup_fn, fd, request, ...)             \
        ({int ret = ioctl(fd, request, __VA_ARGS__);         \
-         ret < 0 ?                                          \
-          tst_brkm(TBROK | TERRNO, cleanup_fn,              \
-                   "ioctl(%i,%s,...) failed", fd, #request) \
-        : ret;})
+         if (ret < 0)                                       \
+             tst_brkm(TBROK | TERRNO, cleanup_fn,           \
+                   "ioctl(%i,%s,...) failed", fd, #request); \
+         ret;})
 
 #endif /* __SAFE_MACROS_H__ */
 #endif /* __TEST_H__ */
diff --git a/include/old/test.h b/include/old/test.h
index d49256073741..93d0c360dfca 100644
--- a/include/old/test.h
+++ b/include/old/test.h
@@ -34,6 +34,10 @@
 #ifndef __TEST_H__
 #define __TEST_H__
 
+#ifdef TST_TEST_H__
+#error newlib already included
+#endif
+
 #include <stdio.h>
 #include <signal.h>
 #include <unistd.h>
@@ -153,9 +157,19 @@ void tst_resm_hexd_(const char *file, const int lineno, int ttype,
 void tst_brkm_(const char *file, const int lineno, int ttype,
        void (*func)(void), const char *arg_fmt, ...)
        __attribute__ ((format (printf, 5, 6))) LTP_ATTRIBUTE_NORETURN;
-#define tst_brkm(ttype, func, arg_fmt, ...) \
-       tst_brkm_(__FILE__, __LINE__, (ttype), (func), \
-                 (arg_fmt), ##__VA_ARGS__)
+
+#ifdef LTPLIB
+#include "ltp_priv.h"
+# define tst_brkm(flags, cleanup, fmt, ...) do { \
+               if (tst_test) \
+                       tst_brk_(__FILE__, __LINE__, flags, fmt, ##__VA_ARGS__); \
+               else \
+                       tst_brkm_(__FILE__, __LINE__, flags, cleanup, fmt, ##__VA_ARGS__); \
+               } while (0)
+#else
+# define tst_brkm(flags, cleanup, fmt, ...) \
+                       tst_brkm_(__FILE__, __LINE__, flags, cleanup, fmt, ##__VA_ARGS__)
+#endif
 
 void tst_require_root(void);
 void tst_exit(void) LTP_ATTRIBUTE_NORETURN;
diff --git a/include/tst_test.h b/include/tst_test.h
index 7dc371ccf095..463418ab2de2 100644
--- a/include/tst_test.h
+++ b/include/tst_test.h
@@ -18,6 +18,10 @@
 #ifndef TST_TEST_H__
 #define TST_TEST_H__
 
+#ifdef __TEST_H__
+#error oldlib already included
+#endif
+
 #include <unistd.h>
 
 #include "tst_common.h"
diff --git a/lib/Makefile b/lib/Makefile
index 663455ee31ad..6b3f6e1917fb 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -24,7 +24,7 @@ top_srcdir            ?= ..
 
 include $(top_srcdir)/include/mk/env_pre.mk
 
-CFLAGS                 += -I.
+CFLAGS                 += -I. -DLTPLIB
 
 ifneq ($(ANDROID),1)
 FILTER_OUT_DIRS                += android_libpthread android_librt


> We would also need to sprinkle the library code with returns as a part
> of the patch if I'm not mistaken.

Agreed, after removing NORETURN there's no way around that.

--

I keep thinking if we shouldn't do those ~30 trivial changes,
and have one simple tst_brkm() macro for both library and tests.

Regards,
Jan

[LTP] [PATCH 1/2] SAFE_MACROS: Redirect to tst_brk_() early

[Cyril Hrubis]

Hi!
> > This sounds good. The only problem we could hit is some common code
> > outside the lib/ directory used in both newlib and oldlib tests. Which
> > was the original reason I put the redirection directly into the
> > tst_brkm_() code...
> 
> Good point, I guess we'd have to do the same as for ltp/lib, that
> is add -DLTPLIB and deal with warnings.
> 
> ---
> 
> Or we don't make tst_brkm_ so strict, we keep it as it is,
> but we find way to prevent newlib users only direct usage
> of tst_brkm() macro:

Sounds good.

> diff --git a/include/old/safe_macros.h b/include/old/safe_macros.h
> index 25001e806c12..96fb0c21876e 100644
> --- a/include/old/safe_macros.h
> +++ b/include/old/safe_macros.h
> @@ -331,10 +331,10 @@ static inline int safe_setrlimit(const char *file, const int lineno,
>  
>  #define SAFE_IOCTL(cleanup_fn, fd, request, ...)             \
>         ({int ret = ioctl(fd, request, __VA_ARGS__);         \
> -         ret < 0 ?                                          \
> -          tst_brkm(TBROK | TERRNO, cleanup_fn,              \
> -                   "ioctl(%i,%s,...) failed", fd, #request) \
> -        : ret;})
> +         if (ret < 0)                                       \
> +             tst_brkm(TBROK | TERRNO, cleanup_fn,           \
> +                   "ioctl(%i,%s,...) failed", fd, #request); \
> +         ret;})
>  
>  #endif /* __SAFE_MACROS_H__ */
>  #endif /* __TEST_H__ */
> diff --git a/include/old/test.h b/include/old/test.h
> index d49256073741..93d0c360dfca 100644
> --- a/include/old/test.h
> +++ b/include/old/test.h
> @@ -34,6 +34,10 @@
>  #ifndef __TEST_H__
>  #define __TEST_H__
>  
> +#ifdef TST_TEST_H__
> +#error newlib already included
> +#endif

I guess that this is worth a separate patch. I would make it more
explicit about what has been included though.

#ifdef TST_TEST_H__
# error tst_test.h already included
#endif

>  #include <stdio.h>
>  #include <signal.h>
>  #include <unistd.h>
> @@ -153,9 +157,19 @@ void tst_resm_hexd_(const char *file, const int lineno, int ttype,
>  void tst_brkm_(const char *file, const int lineno, int ttype,
>         void (*func)(void), const char *arg_fmt, ...)
>         __attribute__ ((format (printf, 5, 6))) LTP_ATTRIBUTE_NORETURN;
> -#define tst_brkm(ttype, func, arg_fmt, ...) \
> -       tst_brkm_(__FILE__, __LINE__, (ttype), (func), \
> -                 (arg_fmt), ##__VA_ARGS__)
> +
> +#ifdef LTPLIB
> +#include "ltp_priv.h"
> +# define tst_brkm(flags, cleanup, fmt, ...) do { \
> +               if (tst_test) \
> +                       tst_brk_(__FILE__, __LINE__, flags, fmt, ##__VA_ARGS__); \
> +               else \
> +                       tst_brkm_(__FILE__, __LINE__, flags, cleanup, fmt, ##__VA_ARGS__); \
> +               } while (0)
> +#else
> +# define tst_brkm(flags, cleanup, fmt, ...) \
> +                       tst_brkm_(__FILE__, __LINE__, flags, cleanup, fmt, ##__VA_ARGS__)
> +#endif
>  
>  void tst_require_root(void);
>  void tst_exit(void) LTP_ATTRIBUTE_NORETURN;
> diff --git a/include/tst_test.h b/include/tst_test.h
> index 7dc371ccf095..463418ab2de2 100644
> --- a/include/tst_test.h
> +++ b/include/tst_test.h
> @@ -18,6 +18,10 @@
>  #ifndef TST_TEST_H__
>  #define TST_TEST_H__
>  
> +#ifdef __TEST_H__
> +#error oldlib already included
> +#endif

Here as well.

>  #include <unistd.h>
>  
>  #include "tst_common.h"
> diff --git a/lib/Makefile b/lib/Makefile
> index 663455ee31ad..6b3f6e1917fb 100644
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -24,7 +24,7 @@ top_srcdir            ?= ..
>  
>  include $(top_srcdir)/include/mk/env_pre.mk
>  
> -CFLAGS                 += -I.
> +CFLAGS                 += -I. -DLTPLIB
>  
>  ifneq ($(ANDROID),1)
>  FILTER_OUT_DIRS                += android_libpthread android_librt
> 
> 
> > We would also need to sprinkle the library code with returns as a part
> > of the patch if I'm not mistaken.
> 
> Agreed, after removing NORETURN there's no way around that.

Can you please have a look at the loop devices patch first? Second patch
there touches the tst_device.c and I would like to have it included
before we start adding the returns there.

> --
> 
> I keep thinking if we shouldn't do those ~30 trivial changes,
> and have one simple tst_brkm() macro for both library and tests.

I would rather keep the old testcases without such changes. I'm afraid
that removing the noreturn attribute may actually break some messed up
test. But maybe I'm just too paranoid.

-- 
Cyril Hrubis
chrubis@suse.cz