Cleanup && Drop redundant space in safe macros

PATCH Superseded Guangwen Feng 2 participants 18 messages Nov 30, 2016 – Jan 4, 2017 Library code

lore

Share & tools

lore

Guangwen Feng

author

2016-11-30 12:12 UTC #0 lore raw

PATCH 1/5 Cleanup && Drop redundant space in safe macros

Signed-off-by Guangwen Feng <[email protected]>

---
 include/tst_safe_macros.h | 22 +++++++++++-----------
 lib/safe_macros.c         | 22 +++++++++++-----------
 2 files changed, 22 insertions(+), 22 deletions(-)

include/tst_safe_macros.h +11 ~~-11~~

index e91dbd2..ffb5446 100644
@@ -146,7 +146,7 @@ static inline int safe_setpgid(const char *file, const int lineno,
 	rval = setpgid(pid, pgid);
 	if (rval) {
 		tst_brk_(file, lineno, TBROK | TERRNO,
-		         "setpgid(%i, %i) failed", pid, pgid);
+			 "setpgid(%i, %i) failed", pid, pgid);
 	}
 
 	return rval;

@@ -292,8 +292,8 @@ static inline void *safe_mmap(const char *file, const int lineno,
 	rval = mmap(addr, length, prot, flags, fd, offset);
 	if (rval == MAP_FAILED) {
 		tst_brk_(file, lineno, TBROK | TERRNO,
-			"mmap(%p,%zu,%d,%d,%d,%ld) failed",
-			addr, length, prot, flags, fd, (long) offset);
+			 "mmap(%p,%zu,%d,%d,%d,%ld) failed",
+			 addr, length, prot, flags, fd, (long) offset);
 	}
 
 	return rval;

@@ -362,7 +362,7 @@ static inline int safe_fstat(const char *file, const int lineno,
 
 	if (rval == -1) {
 		tst_brk_(file, lineno, TBROK | TERRNO,
-			"fstat(%d,%p) failed", fd, buf);
+			 "fstat(%d,%p) failed", fd, buf);
 	}
 
 	return rval;

@@ -379,7 +379,7 @@ static inline int safe_lstat(const char *file, const int lineno,
 
 	if (rval == -1) {
 		tst_brk_(file, lineno, TBROK | TERRNO,
-			"lstat(%s,%p) failed", path, buf);
+			 "lstat(%s,%p) failed", path, buf);
 	}
 
 	return rval;

@@ -396,8 +396,8 @@ static inline off_t safe_lseek(const char *file, const int lineno,
 
 	if (rval == (off_t) -1) {
 		tst_brk_(file, lineno, TBROK | TERRNO,
-			"lseek(%d,%ld,%d) failed",
-			fd, (long)offset, whence);
+			 "lseek(%d,%ld,%d) failed",
+			 fd, (long)offset, whence);
 	}
 
 	return rval;

@@ -414,8 +414,8 @@ static inline int safe_getrlimit(const char *file, const int lineno,
 
 	if (rval == -1) {
 		tst_brk_(file, lineno, TBROK | TERRNO,
-			"getrlimit(%d,%p) failed",
-			resource, rlim);
+			 "getrlimit(%d,%p) failed",
+			 resource, rlim);
 	}
 
 	return rval;

@@ -451,8 +451,8 @@ static inline sighandler_t safe_signal(const char *file, const int lineno,
 
 	if (rval == SIG_ERR) {
 		tst_brk_(file, lineno, TBROK | TERRNO,
-			"signal(%d,%p) failed",
-			signum, handler);
+			 "signal(%d,%p) failed",
+			 signum, handler);
 	}
 
 	return rval;

lib/safe_macros.c +11 ~~-11~~

index 31dc61c..2db362f 100644
@@ -368,7 +368,7 @@ int safe_link(const char *file, const int lineno,
 
 	if (rval == -1) {
 		tst_brkm(TBROK | TERRNO, cleanup_fn,
-		         "%s:%d: link(%s,%s) failed",
+			 "%s:%d: link(%s,%s) failed",
 			 file, lineno, oldpath, newpath);
 	}

@@ -420,7 +420,7 @@ int safe_symlink(const char *file, const int lineno,
 
 	if (rval == -1) {
 		tst_brkm(TBROK | TERRNO, cleanup_fn,
-		         "%s:%d: symlink(%s,%s) failed",
+			 "%s:%d: symlink(%s,%s) failed",
 			 file, lineno, oldpath, newpath);
 	}

@@ -436,7 +436,7 @@ ssize_t safe_write(const char *file, const int lineno, void (cleanup_fn) (void),
 	if (rval == -1 || (len_strict && (size_t)rval != nbyte)) {
 		tst_brkm(TBROK | TERRNO, cleanup_fn,
 			 "%s:%d: write(%d,%p,%zu) failed",
-		         file, lineno, fildes, buf, rval);
+			 file, lineno, fildes, buf, rval);
 	}
 
 	return rval;

@@ -566,8 +566,8 @@ int safe_chown(const char *file, const int lineno, void (cleanup_fn)(void),
 
 	if (rval == -1) {
 		tst_brkm(TBROK | TERRNO, cleanup_fn,
-			"%s:%d: chown(%s,%d,%d) failed",
-			file, lineno, path, owner, group);
+			 "%s:%d: chown(%s,%d,%d) failed",
+			 file, lineno, path, owner, group);
 	}
 
 	return rval;

@@ -582,7 +582,7 @@ int safe_fchown(const char *file, const int lineno, void (cleanup_fn)(void),
 
 	if (rval == -1) {
 		tst_brkm(TBROK | TERRNO, cleanup_fn,
-		         "%s:%d: fchown(%d,%d,%d) failed",
+			 "%s:%d: fchown(%d,%d,%d) failed",
 			 file, lineno, fd, owner, group);
 	}

@@ -657,7 +657,7 @@ int safe_mkfifo(const char *file, const int lineno,
 
 	if (rval == -1) {
 		tst_brkm(TBROK | TERRNO, cleanup_fn,
-		         "%s:%d: mkfifo(%s, 0%o) failed",
+			 "%s:%d: mkfifo(%s, 0%o) failed",
 			 file, lineno, pathname, mode);
 	}

@@ -724,7 +724,7 @@ DIR* safe_opendir(const char *file, const int lineno, void (cleanup_fn)(void),
 
 	if (!rval) {
 		tst_brkm(TBROK | TERRNO, cleanup_fn,
-		         "%s:%d: opendir(%s) failed", file, lineno, name);
+			 "%s:%d: opendir(%s) failed", file, lineno, name);
 	}
 
 	return rval;

@@ -739,7 +739,7 @@ int safe_closedir(const char *file, const int lineno, void (cleanup_fn)(void),
 
 	if (rval) {
 		tst_brkm(TBROK | TERRNO, cleanup_fn,
-		         "%s:%d: closedir(%p) failed", file, lineno, dirp);
+			 "%s:%d: closedir(%p) failed", file, lineno, dirp);
 	}
 
 	return rval;

@@ -756,7 +756,7 @@ struct dirent *safe_readdir(const char *file, const int lineno, void (cleanup_fn
 
 	if (!rval && errno) {
 		tst_brkm(TBROK | TERRNO, cleanup_fn,
-		         "%s:%d: readdir(%p) failed", file, lineno, dirp);
+			 "%s:%d: readdir(%p) failed", file, lineno, dirp);
 	}
 
 	errno = err;

@@ -771,7 +771,7 @@ int safe_getpriority(const char *file, const int lineno, int which, id_t who)
 	rval = getpriority(which, who);
 	if (errno) {
 		tst_brkm(TBROK | TERRNO, NULL,
-		         "%s:%d getpriority(%i, %i) failed",
+			 "%s:%d getpriority(%i, %i) failed",
 			 file, lineno, which, who);
 	}

Signature · 1 line

1.8.4.2

Guangwen Feng

author

2016-11-30 12:12 UTC #1 lore raw

PATCH 2/5 SAFE_MACROS: Add SAFE_SETPRIORITY()

Signed-off-by Guangwen Feng <[email protected]>

---
 include/tst_safe_macros.h |  5 +++++
 lib/safe_macros.c         | 15 +++++++++++++++
 2 files changed, 20 insertions(+)

include/tst_safe_macros.h +5 -0

index ffb5446..9fa0e17 100644
@@ -471,4 +471,9 @@ int safe_getpriority(const char *file, const int lineno, int which, id_t who);
 #define SAFE_GETPRIORITY(which, who) \
 	safe_getpriority(__FILE__, __LINE__, (which), (who))
 
+int safe_setpriority(const char *file, const int lineno, int which, id_t who,
+	int prio);
+#define SAFE_SETPRIORITY(which, who, prio) \
+	safe_setpriority(__FILE__, __LINE__, (which), (who), (prio))
+
 #endif /* SAFE_MACROS_H__ */

lib/safe_macros.c +15 -0

index 2db362f..a109270 100644
@@ -778,3 +778,18 @@ int safe_getpriority(const char *file, const int lineno, int which, id_t who)
 	errno = err;
 	return rval;
 }
+
+int safe_setpriority(const char *file, const int lineno, int which, id_t who,
+	int prio)
+{
+	int rval;
+
+	rval = setpriority(which, who, prio);
+	if (rval) {
+		tst_brkm(TBROK | TERRNO, NULL,
+			 "%s:%d setpriority(%i, %i, %i) failed",
+			 file, lineno, which, who, prio);
+	}
+
+	return rval;
+}

Signature · 1 line

1.8.4.2

Guangwen Feng

author

2016-11-30 12:12 UTC #2 lore raw

PATCH 3/5 SAFE_MACROS: Add SAFE_SYSTEM()

In reply to Guangwen Feng

Signed-off-by Guangwen Feng <[email protected]>

---
 include/tst_safe_macros.h |  4 ++++
 lib/safe_macros.c         | 14 ++++++++++++++
 2 files changed, 18 insertions(+)

include/tst_safe_macros.h +4 -0

index 9fa0e17..2566079 100644
@@ -476,4 +476,8 @@ int safe_setpriority(const char *file, const int lineno, int which, id_t who,
 #define SAFE_SETPRIORITY(which, who, prio) \
 	safe_setpriority(__FILE__, __LINE__, (which), (who), (prio))
 
+int safe_system(const char *file, const int lineno, const char *cmd);
+#define SAFE_SYSTEM(cmd) \
+	safe_system(__FILE__, __LINE__, (cmd))
+
 #endif /* SAFE_MACROS_H__ */

lib/safe_macros.c +14 -0

index a109270..1c89b8c 100644
@@ -793,3 +793,17 @@ int safe_setpriority(const char *file, const int lineno, int which, id_t who,
 
 	return rval;
 }
+
+int safe_system(const char *file, const int lineno, const char *cmd)
+{
+	int rval;
+
+	rval = system(cmd);
+	if (rval == -1 || !WIFEXITED(rval) || WEXITSTATUS(rval)) {
+		tst_brkm(TBROK | TERRNO, NULL,
+			 "%s:%d system(%s) failed",
+			 file, lineno, cmd);
+	}
+
+	return rval;
+}

Signature · 1 line

1.8.4.2

Cyril Hrubis

2016-12-05 14:33 UTC #3 lore raw

PATCH 3/5 SAFE_MACROS: Add SAFE_SYSTEM()

Hi!
NACK to this one. We already have tst_run_cmd() which is way better for this purpose.

Signature · 2 lines

Cyril Hrubis
[email protected]

Guangwen Feng

author

2016-12-06 10:02 UTC #4 lore raw

PATCH 3/5 SAFE_MACROS: Add SAFE_SYSTEM()

Hi!

Quoted 3 lines · Cyril Hrubis

Hi!
NACK to this one. We already have tst_run_cmd() which is way better for this purpose.

I got it, will use tst_run_cmd() instead, thanks very much.

Best Regards,
Guangwen Feng

Guangwen Feng

author

2016-11-30 12:12 UTC #5 lore raw

PATCH 4/5 SAFE_MACROS: Add SAFE_PRCTL()

In reply to Guangwen Feng

Signed-off-by Guangwen Feng <[email protected]>

---
 include/tst_safe_macros.h |  4 ++++
 lib/safe_macros.c         | 20 ++++++++++++++++++++
 2 files changed, 24 insertions(+)

include/tst_safe_macros.h +4 -0

index 2566079..bd60b40 100644
@@ -480,4 +480,8 @@ int safe_system(const char *file, const int lineno, const char *cmd);
 #define SAFE_SYSTEM(cmd) \
 	safe_system(__FILE__, __LINE__, (cmd))
 
+int safe_prctl(const char *file, const int lineno, int opt, ...);
+#define SAFE_PRCTL(opt, ...) \
+	safe_prctl(__FILE__, __LINE__, (opt), ##__VA_ARGS__)
+
 #endif /* SAFE_MACROS_H__ */

lib/safe_macros.c +20 -0

index 1c89b8c..c8c5383 100644
@@ -807,3 +807,23 @@ int safe_system(const char *file, const int lineno, const char *cmd)
 
 	return rval;
 }
+
+int safe_prctl(const char *file, const int lineno, int opt, ...)
+{
+	va_list ap;
+	int rval;
+	unsigned long arg;
+
+	va_start(ap, opt);
+	arg = va_arg(ap, unsigned long);
+	va_end(ap);
+
+	rval = prctl(opt, arg);
+	if (rval < 0) {
+		tst_brkm(TBROK | TERRNO, NULL,
+			 "%s:%d prctl(%i, %lu) failed",
+			 file, lineno, opt, arg);
+	}
+
+	return rval;
+}

Signature · 1 line

1.8.4.2

Cyril Hrubis

2016-12-05 14:40 UTC #6 lore raw

PATCH 4/5 SAFE_MACROS: Add SAFE_PRCTL()

Hi!

Quoted 13 lines

include/tst_safe_macros.h +4 -0

+int safe_prctl(const char *file, const int lineno, int opt, ...);
+#define SAFE_PRCTL(opt, ...) \
+	safe_prctl(__FILE__, __LINE__, (opt), ##__VA_ARGS__)
+
 #endif /* SAFE_MACROS_H__ */

lib/safe_macros.c +0 -0

index 1c89b8c..c8c5383 100644
@@ -807,3 +807,23 @@ int safe_system(const char *file, const int lineno, const char *cmd)
 
 	return rval;
 }

lib/safe_macros.c int safe_system(const char *file, const int lineno, const char *cmd) · line 819

Quoted 10 lines

lib/safe_macros.c +10 -0

+
+int safe_prctl(const char *file, const int lineno, int opt, ...)
+{
+	va_list ap;
+	int rval;
+	unsigned long arg;
+
+	va_start(ap, opt);
+	arg = va_arg(ap, unsigned long);
+	va_end(ap);

This is broken by design. The prctl() can have up to four long arguments but here we use only the first one unconditionally.

I'm not sure how we can fix this easily. Either we can count number of arguments passed to the SAFE_PRCTL() by hacky preprocessor counting[1] and pass it as a first agument.

Or we can switch on the opt to figure out the number of expected arguments, which is ugly as well, but we can at least start small by adding only opts that we use.

[1]
https://groups.google.com/forum/?_escaped_fragment_=topic/comp.std.c/d-6Mj5Lko_s#!topic/comp.std.c/d-6Mj5Lko_s

Quoted 17 lines

lib/safe_macros.c +9 -0

+	rval = prctl(opt, arg);
+	if (rval < 0) {
+		tst_brkm(TBROK | TERRNO, NULL,
+			 "%s:%d prctl(%i, %lu) failed",
+			 file, lineno, opt, arg);
+	}
+
+	return rval;
+}

Signature · 7 lines

1.8.4.2




-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Signature · 2 lines

Cyril Hrubis
[email protected]

Guangwen Feng

author

2016-12-15 10:17 UTC #7 lore raw

PATCH 4/5 SAFE_MACROS: Add SAFE_PRCTL()

Hi!

Quoted 33 lines · Cyril Hrubis

Hi!
Quoted 13 lines
include/tst_safe_macros.h +4 -0
+int safe_prctl(const char *file, const int lineno, int opt, ...);
+#define SAFE_PRCTL(opt, ...) \
+	safe_prctl(__FILE__, __LINE__, (opt), ##__VA_ARGS__)
+
 #endif /* SAFE_MACROS_H__ */
lib/safe_macros.c +0 -0
index 1c89b8c..c8c5383 100644
@@ -807,3 +807,23 @@ int safe_system(const char *file, const int lineno, const char *cmd)
 
 	return rval;
 }
lib/safe_macros.c int safe_system(const char *file, const int lineno, const char *cmd) · line 819
Quoted 10 lines
lib/safe_macros.c +10 -0
+
+int safe_prctl(const char *file, const int lineno, int opt, ...)
+{
+	va_list ap;
+	int rval;
+	unsigned long arg;
+
+	va_start(ap, opt);
+	arg = va_arg(ap, unsigned long);
+	va_end(ap);
This is broken by design. The prctl() can have up to four long arguments but here we use only the first one unconditionally.
I'm not sure how we can fix this easily. Either we can count number of arguments passed to the SAFE_PRCTL() by hacky preprocessor counting[1] and pass it as a first agument.

Quoted 7 lines

Or we can switch on the opt to figure out the number of expected arguments, which is ugly as well, but we can at least start small by adding only opts that we use.
[1]
https://groups.google.com/forum/?_escaped_fragment_=topic/comp.std.c/d-6Mj5Lko_s#!topic/comp.std.c/d-6Mj5Lko_s

Thanks for your review and kindly suggestion.

I see the prctl()'s arguments is variable in prctl.h, just like the fcntl() and ioctl(), so I think maybe I can do it just as fcntl() and ioctl() did?

Best Regards,
Guangwen Feng

Quoted 18 lines

Quoted 17 lines

lib/safe_macros.c +9 -0

+	rval = prctl(opt, arg);
+	if (rval < 0) {
+		tst_brkm(TBROK | TERRNO, NULL,
+			 "%s:%d prctl(%i, %lu) failed",
+			 file, lineno, opt, arg);
+	}
+
+	return rval;
+}

Signature · 7 lines

1.8.4.2




-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Cyril Hrubis

2016-12-15 10:43 UTC #8 lore raw

PATCH 4/5 SAFE_MACROS: Add SAFE_PRCTL()

Hi!

Quoted 2 lines

I see the prctl()'s arguments is variable in prctl.h, just like the fcntl() and ioctl(), so I think maybe I can do it just as fcntl() and ioctl() did?

Ah, missed that. That would work for this case.

Signature · 2 lines

Cyril Hrubis
[email protected]

Guangwen Feng

author

2016-12-15 10:49 UTC #9 lore raw

PATCH v2 1/4 Cleanup && Drop redundant space in safe macros

Signed-off-by Guangwen Feng <[email protected]>

---
 include/tst_safe_macros.h | 22 +++++++++++-----------
 lib/safe_macros.c         | 28 ++++++++++++++--------------
 2 files changed, 25 insertions(+), 25 deletions(-)

include/tst_safe_macros.h +11 ~~-11~~

index d352d5a3..32b2556 100644
@@ -146,7 +146,7 @@ static inline int safe_setpgid(const char *file, const int lineno,
 	rval = setpgid(pid, pgid);
 	if (rval) {
 		tst_brk_(file, lineno, TBROK | TERRNO,
-		         "setpgid(%i, %i) failed", pid, pgid);
+			 "setpgid(%i, %i) failed", pid, pgid);
 	}
 
 	return rval;

@@ -292,8 +292,8 @@ static inline void *safe_mmap(const char *file, const int lineno,
 	rval = mmap(addr, length, prot, flags, fd, offset);
 	if (rval == MAP_FAILED) {
 		tst_brk_(file, lineno, TBROK | TERRNO,
-			"mmap(%p,%zu,%d,%d,%d,%ld) failed",
-			addr, length, prot, flags, fd, (long) offset);
+			 "mmap(%p,%zu,%d,%d,%d,%ld) failed",
+			 addr, length, prot, flags, fd, (long) offset);
 	}
 
 	return rval;

@@ -362,7 +362,7 @@ static inline int safe_fstat(const char *file, const int lineno,
 
 	if (rval == -1) {
 		tst_brk_(file, lineno, TBROK | TERRNO,
-			"fstat(%d,%p) failed", fd, buf);
+			 "fstat(%d,%p) failed", fd, buf);
 	}
 
 	return rval;

@@ -379,7 +379,7 @@ static inline int safe_lstat(const char *file, const int lineno,
 
 	if (rval == -1) {
 		tst_brk_(file, lineno, TBROK | TERRNO,
-			"lstat(%s,%p) failed", path, buf);
+			 "lstat(%s,%p) failed", path, buf);
 	}
 
 	return rval;

@@ -396,8 +396,8 @@ static inline off_t safe_lseek(const char *file, const int lineno,
 
 	if (rval == (off_t) -1) {
 		tst_brk_(file, lineno, TBROK | TERRNO,
-			"lseek(%d,%ld,%d) failed",
-			fd, (long)offset, whence);
+			 "lseek(%d,%ld,%d) failed",
+			 fd, (long)offset, whence);
 	}
 
 	return rval;

@@ -414,8 +414,8 @@ static inline int safe_getrlimit(const char *file, const int lineno,
 
 	if (rval == -1) {
 		tst_brk_(file, lineno, TBROK | TERRNO,
-			"getrlimit(%d,%p) failed",
-			resource, rlim);
+			 "getrlimit(%d,%p) failed",
+			 resource, rlim);
 	}
 
 	return rval;

@@ -451,8 +451,8 @@ static inline sighandler_t safe_signal(const char *file, const int lineno,
 
 	if (rval == SIG_ERR) {
 		tst_brk_(file, lineno, TBROK | TERRNO,
-			"signal(%d,%p) failed",
-			signum, handler);
+			 "signal(%d,%p) failed",
+			 signum, handler);
 	}
 
 	return rval;

lib/safe_macros.c +14 ~~-14~~

index 094e5c7..d5c8753 100644
@@ -369,7 +369,7 @@ int safe_link(const char *file, const int lineno,
 
 	if (rval == -1) {
 		tst_brkm(TBROK | TERRNO, cleanup_fn,
-		         "%s:%d: link(%s,%s) failed",
+			 "%s:%d: link(%s,%s) failed",
 			 file, lineno, oldpath, newpath);
 	}

@@ -421,7 +421,7 @@ int safe_symlink(const char *file, const int lineno,
 
 	if (rval == -1) {
 		tst_brkm(TBROK | TERRNO, cleanup_fn,
-		         "%s:%d: symlink(%s,%s) failed",
+			 "%s:%d: symlink(%s,%s) failed",
 			 file, lineno, oldpath, newpath);
 	}

@@ -437,7 +437,7 @@ ssize_t safe_write(const char *file, const int lineno, void (cleanup_fn) (void),
 	if (rval == -1 || (len_strict && (size_t)rval != nbyte)) {
 		tst_brkm(TBROK | TERRNO, cleanup_fn,
 			 "%s:%d: write(%d,%p,%zu) failed",
-		         file, lineno, fildes, buf, rval);
+			 file, lineno, fildes, buf, rval);
 	}
 
 	return rval;

@@ -567,8 +567,8 @@ int safe_chown(const char *file, const int lineno, void (cleanup_fn)(void),
 
 	if (rval == -1) {
 		tst_brkm(TBROK | TERRNO, cleanup_fn,
-			"%s:%d: chown(%s,%d,%d) failed",
-			file, lineno, path, owner, group);
+			 "%s:%d: chown(%s,%d,%d) failed",
+			 file, lineno, path, owner, group);
 	}
 
 	return rval;

@@ -583,7 +583,7 @@ int safe_fchown(const char *file, const int lineno, void (cleanup_fn)(void),
 
 	if (rval == -1) {
 		tst_brkm(TBROK | TERRNO, cleanup_fn,
-		         "%s:%d: fchown(%d,%d,%d) failed",
+			 "%s:%d: fchown(%d,%d,%d) failed",
 			 file, lineno, fd, owner, group);
 	}

@@ -658,7 +658,7 @@ int safe_mkfifo(const char *file, const int lineno,
 
 	if (rval == -1) {
 		tst_brkm(TBROK | TERRNO, cleanup_fn,
-		         "%s:%d: mkfifo(%s, 0%o) failed",
+			 "%s:%d: mkfifo(%s, 0%o) failed",
 			 file, lineno, pathname, mode);
 	}

@@ -725,7 +725,7 @@ DIR* safe_opendir(const char *file, const int lineno, void (cleanup_fn)(void),
 
 	if (!rval) {
 		tst_brkm(TBROK | TERRNO, cleanup_fn,
-		         "%s:%d: opendir(%s) failed", file, lineno, name);
+			 "%s:%d: opendir(%s) failed", file, lineno, name);
 	}
 
 	return rval;

@@ -740,7 +740,7 @@ int safe_closedir(const char *file, const int lineno, void (cleanup_fn)(void),
 
 	if (rval) {
 		tst_brkm(TBROK | TERRNO, cleanup_fn,
-		         "%s:%d: closedir(%p) failed", file, lineno, dirp);
+			 "%s:%d: closedir(%p) failed", file, lineno, dirp);
 	}
 
 	return rval;

@@ -757,7 +757,7 @@ struct dirent *safe_readdir(const char *file, const int lineno, void (cleanup_fn
 
 	if (!rval && errno) {
 		tst_brkm(TBROK | TERRNO, cleanup_fn,
-		         "%s:%d: readdir(%p) failed", file, lineno, dirp);
+			 "%s:%d: readdir(%p) failed", file, lineno, dirp);
 	}
 
 	errno = err;

@@ -772,7 +772,7 @@ int safe_getpriority(const char *file, const int lineno, int which, id_t who)
 	rval = getpriority(which, who);
 	if (errno) {
 		tst_brkm(TBROK | TERRNO, NULL,
-		         "%s:%d getpriority(%i, %i) failed",
+			 "%s:%d getpriority(%i, %i) failed",
 			 file, lineno, which, who);
 	}

@@ -795,7 +795,7 @@ int safe_setxattr(const char *file, const int lineno, const char *path,
 		}
 
 		tst_brkm(TBROK | TERRNO, NULL, "%s:%d: setxattr() failed",
-			     file, lineno);
+			 file, lineno);
 	}
 
 	return rval;

@@ -816,7 +816,7 @@ int safe_lsetxattr(const char *file, const int lineno, const char *path,
 		}
 
 		tst_brkm(TBROK | TERRNO, NULL, "%s:%d: lsetxattr() failed",
-			     file, lineno);
+			 file, lineno);
 	}
 
 	return rval;

@@ -837,7 +837,7 @@ int safe_fsetxattr(const char *file, const int lineno, int fd, const char *name,
 		}
 
 		tst_brkm(TBROK | TERRNO, NULL, "%s:%d: fsetxattr() failed",
-			     file, lineno);
+			 file, lineno);
 	}
 
 	return rval;

Signature · 1 line

1.8.4.2

Guangwen Feng

author

2016-12-15 10:49 UTC #10 lore raw

PATCH v2 2/4 SAFE_MACROS: Add SAFE_SETPRIORITY()

Signed-off-by Guangwen Feng <[email protected]>

---
 include/tst_safe_macros.h |  5 +++++
 lib/safe_macros.c         | 15 +++++++++++++++
 2 files changed, 20 insertions(+)

include/tst_safe_macros.h +5 -0

index 32b2556..59973b6 100644
@@ -471,6 +471,11 @@ int safe_getpriority(const char *file, const int lineno, int which, id_t who);
 #define SAFE_GETPRIORITY(which, who) \
 	safe_getpriority(__FILE__, __LINE__, (which), (who))
 
+int safe_setpriority(const char *file, const int lineno, int which, id_t who,
+		     int prio);
+#define SAFE_SETPRIORITY(which, who, prio) \
+	safe_setpriority(__FILE__, __LINE__, (which), (who), (prio))
+
 int safe_setxattr(const char *file, const int lineno, const char *path,
             const char *name, const void *value, size_t size, int flags);
 #define SAFE_SETXATTR(path, name, value, size, flags) \

lib/safe_macros.c +15 -0

index d5c8753..65fe27c 100644
@@ -780,6 +780,21 @@ int safe_getpriority(const char *file, const int lineno, int which, id_t who)
 	return rval;
 }
 
+int safe_setpriority(const char *file, const int lineno, int which, id_t who,
+		     int prio)
+{
+	int rval;
+
+	rval = setpriority(which, who, prio);
+	if (rval) {
+		tst_brkm(TBROK | TERRNO, NULL,
+			"%s:%d setpriority(%i, %i, %i) failed",
+			file, lineno, which, who, prio);
+	}
+
+	return rval;
+}
+
 int safe_setxattr(const char *file, const int lineno, const char *path,
 		  const char *name, const void *value, size_t size, int flags)
 {

Signature · 1 line

1.8.4.2

Guangwen Feng

author

2016-12-15 10:49 UTC #11 lore raw

PATCH v2 3/4 SAFE_MACROS: Add SAFE_PRCTL()

In reply to Guangwen Feng

Signed-off-by Guangwen Feng <[email protected]>

---
 include/tst_safe_macros.h | 7 +++++++
 1 file changed, 7 insertions(+)

include/tst_safe_macros.h +7 -0

index 59973b6..1044a34 100644
@@ -278,6 +278,13 @@ static inline ssize_t safe_pwrite(const char *file, const int lineno,
 	            "fcntl(%i,%s,...) failed", fd, #cmd), 0 \
 	 : ret;})
 
+#define SAFE_PRCTL(opt, ...)                                \
+	({int ret = prctl(opt, ##__VA_ARGS__);              \
+	  ret < 0 ?                                         \
+	   tst_brk(TBROK | TERRNO,                          \
+	            "prctl(%s,...) failed", #opt), 0        \
+	 : ret;})
+
 /*
  * following functions are inline because the behaviour may depend on
  * -D_FILE_OFFSET_BITS=64 -DOFF_T=off64_t compile flags

Signature · 1 line

1.8.4.2

Guangwen Feng

author

2016-12-15 10:49 UTC #12 lore raw

PATCH v2 4/4 syscalls/setpriority01: Use new user id to avoid messing system environment

In reply to Guangwen Feng

setpriority(PRIO_USER, root_proc, prio) sets the priorities of all the root processes to the specified value, though this is kind of expected for the test, it impacts the system environment.

Create a new user and set it privileged, then test with the temp user id to ensure the test safety.

Signed-off-by Guangwen Feng <[email protected]>

---
 .../kernel/syscalls/setpriority/setpriority01.c    | 96 +++++++++++++++++++---
 1 file changed, 85 insertions(+), 11 deletions(-)

testcases/kernel/syscalls/setpriority/setpriority01.c +85 ~~-11~~

index e5111d7..83c9c1e 100644
@@ -23,36 +23,50 @@
  */
 
 #include <errno.h>
+#include <pwd.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <sys/capability.h>
+#include <sys/prctl.h>
 #include <sys/resource.h>
 #include <sys/time.h>
+#include <sys/types.h>
+#include <unistd.h>
+
 #include "tst_test.h"
+#include "linux_syscall_numbers.h"
+
+static const char *username = "ltp_setpriority01";
+static int pid, uid;
+static cap_user_header_t header;
+static cap_user_data_t data;
 
 static struct tcase {
 	int which;
+	int *who;
 } tcases[] = {
-	{PRIO_PROCESS},
-	{PRIO_PGRP},
-	{PRIO_USER}
+	{PRIO_PROCESS, &pid},
+	{PRIO_PGRP, &pid},
+	{PRIO_USER, &uid}
 };
 
-static void verify_setpriority(unsigned int n)
+static void setpriority_test(struct tcase *tc)
 {
-	struct tcase *tc = &tcases[n];
 	int new_prio, cur_prio;
 	int failflag = 0;
 
 	for (new_prio = -20; new_prio < 20; new_prio++) {
-		TEST(setpriority(tc->which, 0, new_prio));
+		TEST(setpriority(tc->which, *tc->who, new_prio));
 
 		if (TEST_RETURN != 0) {
 			tst_res(TFAIL | TTERRNO,
-				"setpriority(%d, 0, %d) failed",
-				tc->which, new_prio);
+				"setpriority(%d, %d, %d) failed",
+				tc->which, *tc->who, new_prio);
 			failflag = 1;
 			continue;
 		}
 
-		cur_prio = SAFE_GETPRIORITY(tc->which, 0);
+		cur_prio = SAFE_GETPRIORITY(tc->which, *tc->who);
 
 		if (cur_prio != new_prio) {
 			tst_res(TFAIL, "current priority(%d) and "

@@ -63,14 +77,74 @@ static void verify_setpriority(unsigned int n)
 	}
 
 	if (!failflag) {
-		tst_res(TPASS, "setpriority(%d, 0, -20..19) succeeded",
-			tc->which);
+		tst_res(TPASS, "setpriority(%d, %d, -20..19) succeeded",
+			tc->which, *tc->who);
+	}
+}
+
+static void verify_setpriority(unsigned int n)
+{
+	struct tcase *tc = &tcases[n];
+
+	pid = SAFE_FORK();
+	if (pid == 0) {
+		SAFE_PRCTL(PR_SET_KEEPCAPS, 1);
+
+		SAFE_SETUID(uid);
+		SAFE_SETPGID(0, 0);
+
+		if (tst_syscall(__NR_capset, header, data) == -1)
+			tst_brk(TBROK | TERRNO, "capset() failed");
+
+		TST_CHECKPOINT_WAKE_AND_WAIT(0);
+
+		exit(0);
 	}
+
+	TST_CHECKPOINT_WAIT(0);
+
+	setpriority_test(tc);
+
+	TST_CHECKPOINT_WAKE(0);
+
+	tst_reap_children();
+}
+
+static void setup(void)
+{
+	const char *const cmd_useradd[] = {"useradd", username, NULL};
+	struct passwd *ltpuser;
+
+	tst_run_cmd(cmd_useradd, NULL, NULL, 0);
+
+	ltpuser = SAFE_GETPWNAM(username);
+	uid = ltpuser->pw_uid;
+
+	header = SAFE_MALLOC(sizeof(cap_user_header_t));
+	data = SAFE_MALLOC(sizeof(cap_user_data_t));
+
+	header->version = _LINUX_CAPABILITY_VERSION;
+	header->pid = 0;
+
+	if (tst_syscall(__NR_capget, header, data) == -1)
+		tst_brk(TBROK | TERRNO, "capget() failed");
+}
+
+static void cleanup(void)
+{
+	const char *const cmd_userdel[] = {"userdel", "-r", username, NULL};
+
+	if (tst_run_cmd(cmd_userdel, NULL, NULL, 1))
+		tst_res(TWARN | TERRNO, "'userdel -r %s' failed", username);
 }
 
 static struct tst_test test = {
 	.tid = "setpriority01",
 	.tcnt = ARRAY_SIZE(tcases),
 	.needs_root = 1,
+	.forks_child = 1,
+	.needs_checkpoints = 1,
+	.setup = setup,
+	.cleanup = cleanup,
 	.test = verify_setpriority,
 };

Signature · 1 line

1.8.4.2

Cyril Hrubis

2017-01-04 12:13 UTC #13 lore raw

PATCH v2 4/4 syscalls/setpriority01: Use new user id to avoid messing system environment

Hi!

Quoted 6 lines

setpriority(PRIO_USER, root_proc, prio) sets the priorities of all the root processes to the specified value, though this is kind of expected for the test, it impacts the system environment.
Create a new user and set it privileged, then test with the temp user id to ensure the test safety.

I've dropped all the capability stuff from the patch since the call to setpriority() is now done in the main test process that runs under the root. The child is create just so that we have a process whoose priority will be changed.

Pushed with that change, thanks.

Signature · 2 lines

Cyril Hrubis
[email protected]

Guangwen Feng

author

2016-11-30 12:12 UTC #14 lore raw

PATCH 5/5 syscalls/setpriority01: Use new user id to avoid messing system environment

In reply to Guangwen Feng

setpriority(PRIO_USER, root_proc, prio) sets the priorities of all the root processes to the specified value, though this is kind of expected for the test, it impacts the system environment.

Create a new user and set it privileged, then test with the temp user id to ensure the test safety.

Signed-off-by Guangwen Feng <[email protected]>

---
 .../kernel/syscalls/setpriority/setpriority01.c    | 68 +++++++++++++++++++++-
 1 file changed, 66 insertions(+), 2 deletions(-)

testcases/kernel/syscalls/setpriority/setpriority01.c +66 -2

index e5111d7..0e53784 100644
@@ -23,9 +23,23 @@
  */
 
 #include <errno.h>
+#include <pwd.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <sys/capability.h>
+#include <sys/prctl.h>
 #include <sys/resource.h>
 #include <sys/time.h>
+#include <sys/types.h>
+#include <unistd.h>
+
 #include "tst_test.h"
+#include "linux_syscall_numbers.h"
+
+static char username[32];
+static uid_t uid;
+static struct __user_cap_header_struct header;
+static struct __user_cap_data_struct data;
 
 static struct tcase {
 	int which;

@@ -35,9 +49,8 @@ static struct tcase {
 	{PRIO_USER}
 };
 
-static void verify_setpriority(unsigned int n)
+static void setpriority_test(struct tcase *tc)
 {
-	struct tcase *tc = &tcases[n];
 	int new_prio, cur_prio;
 	int failflag = 0;

@@ -68,9 +81,60 @@ static void verify_setpriority(unsigned int n)
 	}
 }
 
+static void verify_setpriority(unsigned int n)
+{
+	struct tcase *tc = &tcases[n];
+
+	if (!SAFE_FORK()) {
+		SAFE_PRCTL(PR_SET_KEEPCAPS, 1);
+
+		SAFE_SETUID(uid);
+		SAFE_SETPGID(0, 0);
+
+		if (tst_syscall(__NR_capset, &header, &data) == -1)
+			tst_brk(TBROK | TERRNO, "capset() failed");
+
+		setpriority_test(tc);
+
+		exit(0);
+	}
+
+	tst_reap_children();
+}
+
+static void setup(void)
+{
+	char cmd[64];
+	struct passwd *ltpuser;
+
+	sprintf(username, "setpriority01.%d", getpid());
+
+	sprintf(cmd, "useradd %s", username);
+	SAFE_SYSTEM(cmd);
+
+	ltpuser = SAFE_GETPWNAM(username);
+	uid = ltpuser->pw_uid;
+
+	header.version = _LINUX_CAPABILITY_VERSION;
+	header.pid = 0;
+	if (tst_syscall(__NR_capget, &header, &data) == -1)
+		tst_brk(TBROK | TERRNO, "capget() failed");
+}
+
+static void cleanup(void)
+{
+	char cmd[64];
+
+	sprintf(cmd, "userdel -r %s", username);
+	SAFE_SYSTEM(cmd);
+}
+
 static struct tst_test test = {
 	.tid = "setpriority01",
 	.tcnt = ARRAY_SIZE(tcases),
 	.needs_root = 1,
+	.forks_child = 1,
+	.setup = setup,
+	.cleanup = cleanup,
 	.test = verify_setpriority,
 };

Signature · 1 line

1.8.4.2

Cyril Hrubis

2016-12-05 14:45 UTC #15 lore raw

PATCH 5/5 syscalls/setpriority01: Use new user id to avoid messing system environment

Hi!

testcases/kernel/syscalls/setpriority/setpriority01.c line 42

Quoted 7 lines

#include "tst_test.h"

testcases/kernel/syscalls/setpriority/setpriority01.c +6 -0

+#include "linux_syscall_numbers.h"
+
+static char username[32];
+static uid_t uid;
+static struct __user_cap_header_struct header;
+static struct __user_cap_data_struct data;

Are you sure these are the right structure prototypes to use? The double underscore usually means "library internal, do not touch".

Quoted 25 lines

testcases/kernel/syscalls/setpriority/setpriority01.c +10 -2

 	int which;

@@ -35,9 +49,8 @@ static struct tcase {
 	{PRIO_USER}
 };
 
-static void verify_setpriority(unsigned int n)
+static void setpriority_test(struct tcase *tc)
 {
-	struct tcase *tc = &tcases[n];
 	int new_prio, cur_prio;
 	int failflag = 0;

@@ -68,9 +81,60 @@ static void verify_setpriority(unsigned int n)
 	}
 }
 
+static void verify_setpriority(unsigned int n)
+{
+	struct tcase *tc = &tcases[n];
+
+	if (!SAFE_FORK()) {
+		SAFE_PRCTL(PR_SET_KEEPCAPS, 1);
+
+		SAFE_SETUID(uid);
+		SAFE_SETPGID(0, 0);

testcases/kernel/syscalls/setpriority/setpriority01.c static void verify_setpriority(unsigned int n) · line 102

Quoted 10 lines

testcases/kernel/syscalls/setpriority/setpriority01.c +10 -0

+
+		if (tst_syscall(__NR_capset, &header, &data) == -1)
+			tst_brk(TBROK | TERRNO, "capset() failed");
+
+		setpriority_test(tc);
+
+		exit(0);
+	}
+
+	tst_reap_children();

Hmm, why do we run the actuall testcase in the child here? That complicates the whole test far more than it should have.

We can instead just run pause() in the child and run the setpriority() call in the parent with right pid/group/uid. Or did I miss something?

Quoted 46 lines

testcases/kernel/syscalls/setpriority/setpriority01.c +32 -0

+}
+
+static void setup(void)
+{
+	char cmd[64];
+	struct passwd *ltpuser;
+
+	sprintf(username, "setpriority01.%d", getpid());
+
+	sprintf(cmd, "useradd %s", username);
+	SAFE_SYSTEM(cmd);
+
+	ltpuser = SAFE_GETPWNAM(username);
+	uid = ltpuser->pw_uid;
+
+	header.version = _LINUX_CAPABILITY_VERSION;
+	header.pid = 0;
+	if (tst_syscall(__NR_capget, &header, &data) == -1)
+		tst_brk(TBROK | TERRNO, "capget() failed");
+}
+
+static void cleanup(void)
+{
+	char cmd[64];
+
+	sprintf(cmd, "userdel -r %s", username);
+	SAFE_SYSTEM(cmd);
+}
+
 static struct tst_test test = {
 	.tid = "setpriority01",
 	.tcnt = ARRAY_SIZE(tcases),
 	.needs_root = 1,
+	.forks_child = 1,
+	.setup = setup,
+	.cleanup = cleanup,
 	.test = verify_setpriority,
 };

Signature · 7 lines

1.8.4.2




-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Signature · 2 lines

Cyril Hrubis
[email protected]

Guangwen Feng

author

2016-12-06 10:27 UTC #16 lore raw

PATCH 5/5 syscalls/setpriority01: Use new user id to avoid messing system environment

Hi!

Quoted 12 lines · Cyril Hrubis

Hi!
testcases/kernel/syscalls/setpriority/setpriority01.c line 42
Quoted 7 lines
#include "tst_test.h"
testcases/kernel/syscalls/setpriority/setpriority01.c +6 -0
+#include "linux_syscall_numbers.h"
+
+static char username[32];
+static uid_t uid;
+static struct __user_cap_header_struct header;
+static struct __user_cap_data_struct data;
Are you sure these are the right structure prototypes to use? The double underscore usually means "library internal, do not touch".

It's my mistake, thanks for review, I will correct this.

Quoted 39 lines

Quoted 25 lines

testcases/kernel/syscalls/setpriority/setpriority01.c +10 -2

 	int which;

@@ -35,9 +49,8 @@ static struct tcase {
 	{PRIO_USER}
 };
 
-static void verify_setpriority(unsigned int n)
+static void setpriority_test(struct tcase *tc)
 {
-	struct tcase *tc = &tcases[n];
 	int new_prio, cur_prio;
 	int failflag = 0;

@@ -68,9 +81,60 @@ static void verify_setpriority(unsigned int n)
 	}
 }
 
+static void verify_setpriority(unsigned int n)
+{
+	struct tcase *tc = &tcases[n];
+
+	if (!SAFE_FORK()) {
+		SAFE_PRCTL(PR_SET_KEEPCAPS, 1);
+
+		SAFE_SETUID(uid);
+		SAFE_SETPGID(0, 0);

testcases/kernel/syscalls/setpriority/setpriority01.c static void verify_setpriority(unsigned int n) · line 102

Quoted 10 lines

testcases/kernel/syscalls/setpriority/setpriority01.c +10 -0

+
+		if (tst_syscall(__NR_capset, &header, &data) == -1)
+			tst_brk(TBROK | TERRNO, "capset() failed");
+
+		setpriority_test(tc);
+
+		exit(0);
+	}
+
+	tst_reap_children();

Hmm, why do we run the actuall testcase in the child here? That complicates the whole test far more than it should have.

Quoted 3 lines

We can instead just run pause() in the child and run the setpriority() call in the parent with right pid/group/uid. Or did I miss something?

Sorry, if we do the actual test in the parent, we have to do the process synchronization, because parent needs child to be ready (prctl, setuid...) and after every test, child's pause() needs to be waken up by parent, so I feel it's more complicated by comparison...

Best Regards,
Guangwen Feng

Quoted 47 lines

Quoted 46 lines

testcases/kernel/syscalls/setpriority/setpriority01.c +32 -0

+}
+
+static void setup(void)
+{
+	char cmd[64];
+	struct passwd *ltpuser;
+
+	sprintf(username, "setpriority01.%d", getpid());
+
+	sprintf(cmd, "useradd %s", username);
+	SAFE_SYSTEM(cmd);
+
+	ltpuser = SAFE_GETPWNAM(username);
+	uid = ltpuser->pw_uid;
+
+	header.version = _LINUX_CAPABILITY_VERSION;
+	header.pid = 0;
+	if (tst_syscall(__NR_capget, &header, &data) == -1)
+		tst_brk(TBROK | TERRNO, "capget() failed");
+}
+
+static void cleanup(void)
+{
+	char cmd[64];
+
+	sprintf(cmd, "userdel -r %s", username);
+	SAFE_SYSTEM(cmd);
+}
+
 static struct tst_test test = {
 	.tid = "setpriority01",
 	.tcnt = ARRAY_SIZE(tcases),
 	.needs_root = 1,
+	.forks_child = 1,
+	.setup = setup,
+	.cleanup = cleanup,
 	.test = verify_setpriority,
 };

Signature · 7 lines

1.8.4.2




-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Cyril Hrubis

2016-12-06 10:34 UTC #17 lore raw

PATCH 5/5 syscalls/setpriority01: Use new user id to avoid messing system environment

Hi!

Quoted 11 lines

Quoted 6 lines
Hmm, why do we run the actuall testcase in the child here? That complicates the whole test far more than it should have.
We can instead just run pause() in the child and run the setpriority() call in the parent with right pid/group/uid. Or did I miss something?
Sorry, if we do the actual test in the parent, we have to do the process synchronization, because parent needs child to be ready (prctl, setuid...) and after every test, child's pause() needs to be waken up by parent, so I feel it's more complicated by comparison...

Since we have the checkpoint synchronization in the test library this is just a matter of calling TST_CHECKPOINT_WAIT() in the parent and TST_CHECKPOINT_WAKE_AND_WAIT() in the child then finally TST_CHECKPOINT_WAKE() in the parent.

Signature · 2 lines

Cyril Hrubis
[email protected]