[PATCH] selftests: capabilities: convert error output to TAP13 ksft framework

[PATCH] selftests: capabilities: convert error output to TAP13 ksft framework

[Shuah Khan]

Convert errx() and err() usage to appropriate TAP13 ksft API.

Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
---
 tools/testing/selftests/capabilities/test_execve.c | 105 ++++++++++++---------
 .../testing/selftests/capabilities/validate_cap.c  |   9 +-
 2 files changed, 65 insertions(+), 49 deletions(-)

diff --git a/tools/testing/selftests/capabilities/test_execve.c b/tools/testing/selftests/capabilities/test_execve.c
index 7c38233292b0..cf6778441381 100644
--- a/tools/testing/selftests/capabilities/test_execve.c
+++ b/tools/testing/selftests/capabilities/test_execve.c
@@ -1,7 +1,6 @@
 #define _GNU_SOURCE
 
 #include <cap-ng.h>
-#include <err.h>
 #include <linux/capability.h>
 #include <stdbool.h>
 #include <string.h>
@@ -39,29 +38,32 @@ static void vmaybe_write_file(bool enoent_ok, char *filename, char *fmt, va_list
 	int buf_len;
 
 	buf_len = vsnprintf(buf, sizeof(buf), fmt, ap);
-	if (buf_len < 0) {
-		err(1, "vsnprintf failed");
-	}
-	if (buf_len >= sizeof(buf)) {
-		errx(1, "vsnprintf output truncated");
-	}
+	if (buf_len < 0)
+		ksft_exit_fail_msg("vsnprintf failed - %s\n", strerror(errno));
+
+	if (buf_len >= sizeof(buf))
+		ksft_exit_fail_msg("vsnprintf output truncated\n");
+
 
 	fd = open(filename, O_WRONLY);
 	if (fd < 0) {
 		if ((errno == ENOENT) && enoent_ok)
 			return;
-		err(1, "open of %s failed", filename);
+		ksft_exit_fail_msg("open of %s failed - %s\n",
+					filename, strerror(errno));
 	}
 	written = write(fd, buf, buf_len);
 	if (written != buf_len) {
 		if (written >= 0) {
-			errx(1, "short write to %s", filename);
+			ksft_exit_fail_msg("short write to %s\n", filename);
 		} else {
-			err(1, "write to %s failed", filename);
+			ksft_exit_fail_msg("write to %s failed - %s\n",
+						filename, strerror(errno));
 		}
 	}
 	if (close(fd) != 0) {
-		err(1, "close of %s failed", filename);
+		ksft_exit_fail_msg("close of %s failed - %s\n",
+					filename, strerror(errno));
 	}
 }
 
@@ -100,9 +102,10 @@ static bool create_and_enter_ns(uid_t inner_uid)
 	if (unshare(CLONE_NEWNS) == 0) {
 		ksft_print_msg("[NOTE]\tUsing global UIDs for tests\n");
 		if (prctl(PR_SET_KEEPCAPS, 1, 0, 0, 0) != 0)
-			err(1, "PR_SET_KEEPCAPS");
+			ksft_exit_fail_msg("PR_SET_KEEPCAPS - %s\n",
+						strerror(errno));
 		if (setresuid(inner_uid, inner_uid, -1) != 0)
-			err(1, "setresuid");
+			ksft_exit_fail_msg("setresuid - %s\n", strerror(errno));
 
 		// Re-enable effective caps
 		capng_get_caps_process();
@@ -110,7 +113,8 @@ static bool create_and_enter_ns(uid_t inner_uid)
 			if (capng_have_capability(CAPNG_PERMITTED, i))
 				capng_update(CAPNG_ADD, CAPNG_EFFECTIVE, i);
 		if (capng_apply(CAPNG_SELECT_CAPS) != 0)
-			err(1, "capng_apply");
+			ksft_exit_fail_msg(
+					"capng_apply - %s\n", strerror(errno));
 
 		have_outer_privilege = true;
 	} else if (unshare(CLONE_NEWUSER | CLONE_NEWNS) == 0) {
@@ -121,11 +125,12 @@ static bool create_and_enter_ns(uid_t inner_uid)
 
 		have_outer_privilege = false;
 	} else {
-		errx(1, "must be root or be able to create a userns");
+		ksft_exit_skip("must be root or be able to create a userns\n");
 	}
 
 	if (mount("none", "/", NULL, MS_REC | MS_PRIVATE, NULL) != 0)
-		err(1, "remount everything private");
+		ksft_exit_fail_msg("remount everything private - %s\n",
+					strerror(errno));
 
 	return have_outer_privilege;
 }
@@ -134,20 +139,22 @@ static void chdir_to_tmpfs(void)
 {
 	char cwd[PATH_MAX];
 	if (getcwd(cwd, sizeof(cwd)) != cwd)
-		err(1, "getcwd");
+		ksft_exit_fail_msg("getcwd - %s\n", strerror(errno));
 
 	if (mount("private_tmp", ".", "tmpfs", 0, "mode=0777") != 0)
-		err(1, "mount private tmpfs");
+		ksft_exit_fail_msg("mount private tmpfs - %s\n",
+					strerror(errno));
 
 	if (chdir(cwd) != 0)
-		err(1, "chdir to private tmpfs");
+		ksft_exit_fail_msg("chdir to private tmpfs - %s\n",
+					strerror(errno));
 }
 
 static void copy_fromat_to(int fromfd, const char *fromname, const char *toname)
 {
 	int from = openat(fromfd, fromname, O_RDONLY);
 	if (from == -1)
-		err(1, "open copy source");
+		ksft_exit_fail_msg("open copy source - %s\n", strerror(errno));
 
 	int to = open(toname, O_CREAT | O_WRONLY | O_EXCL, 0700);
 
@@ -157,10 +164,11 @@ static void copy_fromat_to(int fromfd, const char *fromname, const char *toname)
 		if (sz == 0)
 			break;
 		if (sz < 0)
-			err(1, "read");
+			ksft_exit_fail_msg("read - %s\n", strerror(errno));
 
 		if (write(to, buf, sz) != sz)
-			err(1, "write");	/* no short writes on tmpfs */
+			/* no short writes on tmpfs */
+			ksft_exit_fail_msg("write - %s\n", strerror(errno));
 	}
 
 	close(from);
@@ -189,7 +197,8 @@ static bool fork_wait(void)
 		}
 		return false;
 	} else {
-		err(1, "fork");
+		ksft_exit_fail_msg("fork - %s\n", strerror(errno));
+		return false;
 	}
 }
 
@@ -199,7 +208,7 @@ static void exec_other_validate_cap(const char *name,
 	execl(name, name, (eff ? "1" : "0"),
 	      (perm ? "1" : "0"), (inh ? "1" : "0"), (ambient ? "1" : "0"),
 	      NULL);
-	err(1, "execl");
+	ksft_exit_fail_msg("execl - %s\n", strerror(errno));
 }
 
 static void exec_validate_cap(bool eff, bool perm, bool inh, bool ambient)
@@ -213,7 +222,8 @@ static int do_tests(int uid, const char *our_path)
 
 	int ourpath_fd = open(our_path, O_RDONLY | O_DIRECTORY);
 	if (ourpath_fd == -1)
-		err(1, "open '%s'", our_path);
+		ksft_exit_fail_msg("open '%s' - %s\n",
+					our_path, strerror(errno));
 
 	chdir_to_tmpfs();
 
@@ -225,30 +235,30 @@ static int do_tests(int uid, const char *our_path)
 		copy_fromat_to(ourpath_fd, "validate_cap",
 			       "validate_cap_suidroot");
 		if (chown("validate_cap_suidroot", 0, -1) != 0)
-			err(1, "chown");
+			ksft_exit_fail_msg("chown - %s\n", strerror(errno));
 		if (chmod("validate_cap_suidroot", S_ISUID | 0700) != 0)
-			err(1, "chmod");
+			ksft_exit_fail_msg("chmod - %s\n", strerror(errno));
 
 		copy_fromat_to(ourpath_fd, "validate_cap",
 			       "validate_cap_suidnonroot");
 		if (chown("validate_cap_suidnonroot", uid + 1, -1) != 0)
-			err(1, "chown");
+			ksft_exit_fail_msg("chown - %s\n", strerror(errno));
 		if (chmod("validate_cap_suidnonroot", S_ISUID | 0700) != 0)
-			err(1, "chmod");
+			ksft_exit_fail_msg("chmod - %s\n", strerror(errno));
 
 		copy_fromat_to(ourpath_fd, "validate_cap",
 			       "validate_cap_sgidroot");
 		if (chown("validate_cap_sgidroot", -1, 0) != 0)
-			err(1, "chown");
+			ksft_exit_fail_msg("chown - %s\n", strerror(errno));
 		if (chmod("validate_cap_sgidroot", S_ISGID | 0710) != 0)
-			err(1, "chmod");
+			ksft_exit_fail_msg("chmod - %s\n", strerror(errno));
 
 		copy_fromat_to(ourpath_fd, "validate_cap",
 			       "validate_cap_sgidnonroot");
 		if (chown("validate_cap_sgidnonroot", -1, gid + 1) != 0)
-			err(1, "chown");
+			ksft_exit_fail_msg("chown - %s\n", strerror(errno));
 		if (chmod("validate_cap_sgidnonroot", S_ISGID | 0710) != 0)
-			err(1, "chmod");
+			ksft_exit_fail_msg("chmod - %s\n", strerror(errno));
 	}
 
 	capng_get_caps_process();
@@ -256,7 +266,7 @@ static int do_tests(int uid, const char *our_path)
 	/* Make sure that i starts out clear */
 	capng_update(CAPNG_DROP, CAPNG_INHERITABLE, CAP_NET_BIND_SERVICE);
 	if (capng_apply(CAPNG_SELECT_CAPS) != 0)
-		err(1, "capng_apply");
+		ksft_exit_fail_msg("capng_apply - %s\n", strerror(errno));
 
 	if (uid == 0) {
 		ksft_print_msg("[RUN]\tRoot => ep\n");
@@ -287,7 +297,7 @@ static int do_tests(int uid, const char *our_path)
 	capng_update(CAPNG_DROP, CAPNG_PERMITTED, CAP_NET_RAW);
 	capng_update(CAPNG_DROP, CAPNG_EFFECTIVE, CAP_NET_RAW);
 	if (capng_apply(CAPNG_SELECT_CAPS) != 0)
-		err(1, "capng_apply");
+		ksft_exit_fail_msg("capng_apply - %s\n", strerror(errno));
 	if (prctl(PR_CAP_AMBIENT, PR_CAP_AMBIENT_RAISE, CAP_NET_RAW, 0, 0, 0) != -1 || errno != EPERM) {
 		ksft_test_result_fail(
 			"PR_CAP_AMBIENT_RAISE should have failed on a non-permitted cap\n");
@@ -298,7 +308,7 @@ static int do_tests(int uid, const char *our_path)
 
 	capng_update(CAPNG_ADD, CAPNG_INHERITABLE, CAP_NET_BIND_SERVICE);
 	if (capng_apply(CAPNG_SELECT_CAPS) != 0)
-		err(1, "capng_apply");
+		ksft_exit_fail_msg("capng_apply - %s\n", strerror(errno));
 	if (prctl(PR_CAP_AMBIENT, PR_CAP_AMBIENT_RAISE, CAP_NET_BIND_SERVICE, 0, 0, 0) != 0) {
 		ksft_test_result_fail(
 			"PR_CAP_AMBIENT_RAISE should have succeeded\n");
@@ -312,7 +322,8 @@ static int do_tests(int uid, const char *our_path)
 	}
 
 	if (prctl(PR_CAP_AMBIENT, PR_CAP_AMBIENT_CLEAR_ALL, 0, 0, 0, 0) != 0)
-		err(1, "PR_CAP_AMBIENT_CLEAR_ALL");
+		ksft_exit_fail_msg("PR_CAP_AMBIENT_CLEAR_ALL - %s\n",
+					strerror(errno));
 
 	if (prctl(PR_CAP_AMBIENT, PR_CAP_AMBIENT_IS_SET, CAP_NET_BIND_SERVICE, 0, 0, 0) != 0) {
 		ksft_test_result_fail(
@@ -321,11 +332,12 @@ static int do_tests(int uid, const char *our_path)
 	}
 
 	if (prctl(PR_CAP_AMBIENT, PR_CAP_AMBIENT_RAISE, CAP_NET_BIND_SERVICE, 0, 0, 0) != 0)
-		err(1, "PR_CAP_AMBIENT_RAISE");
+		ksft_exit_fail_msg("PR_CAP_AMBIENT_RAISE - %s\n",
+					strerror(errno));
 
 	capng_update(CAPNG_DROP, CAPNG_INHERITABLE, CAP_NET_BIND_SERVICE);
 	if (capng_apply(CAPNG_SELECT_CAPS) != 0)
-		err(1, "capng_apply");
+		ksft_exit_fail_msg("capng_apply - %s\n", strerror(errno));
 
 	if (prctl(PR_CAP_AMBIENT, PR_CAP_AMBIENT_IS_SET, CAP_NET_BIND_SERVICE, 0, 0, 0) != 0) {
 		ksft_test_result_fail("Dropping I should have dropped A\n");
@@ -336,7 +348,7 @@ static int do_tests(int uid, const char *our_path)
 
 	capng_update(CAPNG_ADD, CAPNG_INHERITABLE, CAP_NET_BIND_SERVICE);
 	if (capng_apply(CAPNG_SELECT_CAPS) != 0)
-		err(1, "capng_apply");
+		ksft_exit_fail_msg("capng_apply - %s\n", strerror(errno));
 	if (uid == 0) {
 		ksft_print_msg("[RUN]\tRoot +i => eip\n");
 		if (fork_wait())
@@ -348,7 +360,8 @@ static int do_tests(int uid, const char *our_path)
 	}
 
 	if (prctl(PR_CAP_AMBIENT, PR_CAP_AMBIENT_RAISE, CAP_NET_BIND_SERVICE, 0, 0, 0) != 0)
-		err(1, "PR_CAP_AMBIENT_RAISE");
+		ksft_exit_fail_msg("PR_CAP_AMBIENT_RAISE - %s\n",
+					strerror(errno));
 
 	ksft_print_msg("[RUN]\tUID %d +ia => eipa\n", uid);
 	if (fork_wait())
@@ -381,7 +394,8 @@ static int do_tests(int uid, const char *our_path)
 			ksft_print_msg(
 				"[RUN]\tRoot, gid != 0, +ia, sgidroot => eip\n");
 			if (setresgid(1, 1, 1) != 0)
-				err(1, "setresgid");
+				ksft_exit_fail_msg("setresgid - %s\n",
+							strerror(errno));
 			exec_other_validate_cap("./validate_cap_sgidroot",
 						true, true, true, false);
 		}
@@ -399,7 +413,8 @@ static int do_tests(int uid, const char *our_path)
 		if (fork_wait()) {
 			ksft_print_msg("[RUN]\tNon-root +ia, sgidroot => i\n");
 			if (setresgid(1, 1, 1) != 0)
-				err(1, "setresgid");
+				ksft_exit_fail_msg("setresgid - %s\n",
+							strerror(errno));
 			exec_other_validate_cap("./validate_cap_sgidroot",
 						false, false, true, false);
 		}
@@ -419,11 +434,11 @@ int main(int argc, char **argv)
 	/* Find our path */
 	tmp1 = strdup(argv[0]);
 	if (!tmp1)
-		err(1, "strdup");
+		ksft_exit_fail_msg("strdup - %s\n", strerror(errno));
 	tmp2 = dirname(tmp1);
 	our_path = strdup(tmp2);
 	if (!our_path)
-		err(1, "strdup");
+		ksft_exit_fail_msg("strdup - %s\n", strerror(errno));
 	free(tmp1);
 
 	mpid = getpid();
diff --git a/tools/testing/selftests/capabilities/validate_cap.c b/tools/testing/selftests/capabilities/validate_cap.c
index 9fd4345a8987..694cd73d4493 100644
--- a/tools/testing/selftests/capabilities/validate_cap.c
+++ b/tools/testing/selftests/capabilities/validate_cap.c
@@ -1,5 +1,4 @@
 #include <cap-ng.h>
-#include <err.h>
 #include <linux/capability.h>
 #include <stdbool.h>
 #include <string.h>
@@ -27,8 +26,10 @@ static bool bool_arg(char **argv, int i)
 		return false;
 	else if (!strcmp(argv[i], "1"))
 		return true;
-	else
-		errx(1, "wrong argv[%d]", i);
+	else {
+		ksft_exit_fail_msg("wrong argv[%d]\n", i);
+		return false;
+	}
 }
 
 int main(int argc, char **argv)
@@ -41,7 +42,7 @@ int main(int argc, char **argv)
 	 */
 
 	if (argc != 5)
-		errx(1, "wrong argc");
+		ksft_exit_fail_msg("wrong argc\n");
 
 #ifdef HAVE_GETAUXVAL
 	if (getauxval(AT_SECURE))
-- 
2.11.0

Re: [PATCH] selftests: capabilities: convert error output to TAP13 ksft framework

[Andy Lutomirski]

On Wed, Jul 26, 2017 at 2:18 PM, Shuah Khan <shuahkh@osg.samsung.com> wrote:
> Convert errx() and err() usage to appropriate TAP13 ksft API.
>
> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
> ---
>  tools/testing/selftests/capabilities/test_execve.c | 105 ++++++++++++---------
>  .../testing/selftests/capabilities/validate_cap.c  |   9 +-
>  2 files changed, 65 insertions(+), 49 deletions(-)
>
> diff --git a/tools/testing/selftests/capabilities/test_execve.c b/tools/testing/selftests/capabilities/test_execve.c
> index 7c38233292b0..cf6778441381 100644
> --- a/tools/testing/selftests/capabilities/test_execve.c
> +++ b/tools/testing/selftests/capabilities/test_execve.c
> @@ -1,7 +1,6 @@
>  #define _GNU_SOURCE
>
>  #include <cap-ng.h>
> -#include <err.h>
>  #include <linux/capability.h>
>  #include <stdbool.h>
>  #include <string.h>
> @@ -39,29 +38,32 @@ static void vmaybe_write_file(bool enoent_ok, char *filename, char *fmt, va_list
>         int buf_len;
>
>         buf_len = vsnprintf(buf, sizeof(buf), fmt, ap);
> -       if (buf_len < 0) {
> -               err(1, "vsnprintf failed");
> -       }
> -       if (buf_len >= sizeof(buf)) {
> -               errx(1, "vsnprintf output truncated");
> -       }
> +       if (buf_len < 0)
> +               ksft_exit_fail_msg("vsnprintf failed - %s\n", strerror(errno));

Could this not be a hypothetical ksft_exit_fail_msg_err or similar?
Or a shorter name like ksft_fatal_err()?

Re: [PATCH] selftests: capabilities: convert error output to TAP13 ksft framework

[Shuah Khan]

On 07/27/2017 12:50 PM, Andy Lutomirski wrote:
> On Wed, Jul 26, 2017 at 2:18 PM, Shuah Khan <shuahkh@osg.samsung.com> wrote:
>> Convert errx() and err() usage to appropriate TAP13 ksft API.
>>
>> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
>> ---
>>  tools/testing/selftests/capabilities/test_execve.c | 105 ++++++++++++---------
>>  .../testing/selftests/capabilities/validate_cap.c  |   9 +-
>>  2 files changed, 65 insertions(+), 49 deletions(-)
>>
>> diff --git a/tools/testing/selftests/capabilities/test_execve.c b/tools/testing/selftests/capabilities/test_execve.c
>> index 7c38233292b0..cf6778441381 100644
>> --- a/tools/testing/selftests/capabilities/test_execve.c
>> +++ b/tools/testing/selftests/capabilities/test_execve.c
>> @@ -1,7 +1,6 @@
>>  #define _GNU_SOURCE
>>
>>  #include <cap-ng.h>
>> -#include <err.h>
>>  #include <linux/capability.h>
>>  #include <stdbool.h>
>>  #include <string.h>
>> @@ -39,29 +38,32 @@ static void vmaybe_write_file(bool enoent_ok, char *filename, char *fmt, va_list
>>         int buf_len;
>>
>>         buf_len = vsnprintf(buf, sizeof(buf), fmt, ap);
>> -       if (buf_len < 0) {
>> -               err(1, "vsnprintf failed");
>> -       }
>> -       if (buf_len >= sizeof(buf)) {
>> -               errx(1, "vsnprintf output truncated");
>> -       }
>> +       if (buf_len < 0)
>> +               ksft_exit_fail_msg("vsnprintf failed - %s\n", strerror(errno));
> 
> Could this not be a hypothetical ksft_exit_fail_msg_err or similar?
> Or a shorter name like ksft_fatal_err()?
> 
> 

Is there a reason to add _err() suffix?

ksft_exit_fail_msg() is a generic routine for a test to exit
with a test failure and print a message. The message doesn't
necessarily need to be a standard error message such as the
one err() or errx() or strerror() generate.

In some cases test could fail with a standard error condition,
but not always. In that context, it doesn't make sense to add
_err suffix. I leveraged this generic function to replace err()
and errx() usages adding strerror() not loose the important
information.

thanks,
-- Shuah

Re: [PATCH] selftests: capabilities: convert error output to TAP13 ksft framework

[Andy Lutomirski]

On Thu, Jul 27, 2017 at 1:32 PM, Shuah Khan <shuah@kernel.org> wrote:
> On 07/27/2017 12:50 PM, Andy Lutomirski wrote:
>> On Wed, Jul 26, 2017 at 2:18 PM, Shuah Khan <shuahkh@osg.samsung.com> wrote:
>>> Convert errx() and err() usage to appropriate TAP13 ksft API.
>>>
>>> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
>>> ---
>>>  tools/testing/selftests/capabilities/test_execve.c | 105 ++++++++++++---------
>>>  .../testing/selftests/capabilities/validate_cap.c  |   9 +-
>>>  2 files changed, 65 insertions(+), 49 deletions(-)
>>>
>>> diff --git a/tools/testing/selftests/capabilities/test_execve.c b/tools/testing/selftests/capabilities/test_execve.c
>>> index 7c38233292b0..cf6778441381 100644
>>> --- a/tools/testing/selftests/capabilities/test_execve.c
>>> +++ b/tools/testing/selftests/capabilities/test_execve.c
>>> @@ -1,7 +1,6 @@
>>>  #define _GNU_SOURCE
>>>
>>>  #include <cap-ng.h>
>>> -#include <err.h>
>>>  #include <linux/capability.h>
>>>  #include <stdbool.h>
>>>  #include <string.h>
>>> @@ -39,29 +38,32 @@ static void vmaybe_write_file(bool enoent_ok, char *filename, char *fmt, va_list
>>>         int buf_len;
>>>
>>>         buf_len = vsnprintf(buf, sizeof(buf), fmt, ap);
>>> -       if (buf_len < 0) {
>>> -               err(1, "vsnprintf failed");
>>> -       }
>>> -       if (buf_len >= sizeof(buf)) {
>>> -               errx(1, "vsnprintf output truncated");
>>> -       }
>>> +       if (buf_len < 0)
>>> +               ksft_exit_fail_msg("vsnprintf failed - %s\n", strerror(errno));
>>
>> Could this not be a hypothetical ksft_exit_fail_msg_err or similar?
>> Or a shorter name like ksft_fatal_err()?
>>
>>
>
> Is there a reason to add _err() suffix?
>
> ksft_exit_fail_msg() is a generic routine for a test to exit
> with a test failure and print a message. The message doesn't
> necessarily need to be a standard error message such as the
> one err() or errx() or strerror() generate.
>
> In some cases test could fail with a standard error condition,
> but not always. In that context, it doesn't make sense to add
> _err suffix. I leveraged this generic function to replace err()
> and errx() usages adding strerror() not loose the important
> information.

The idea behind the _err version is to avoid the extra typing to
report errno.  I suppose you could always report errno, but there are
contexts where errno is meaningless or garbage.

Re: [PATCH] selftests: capabilities: convert error output to TAP13 ksft framework

[Shuah Khan]

On 07/27/2017 08:13 PM, Andy Lutomirski wrote:
> On Thu, Jul 27, 2017 at 1:32 PM, Shuah Khan <shuah@kernel.org> wrote:
>> On 07/27/2017 12:50 PM, Andy Lutomirski wrote:
>>> On Wed, Jul 26, 2017 at 2:18 PM, Shuah Khan <shuahkh@osg.samsung.com> wrote:
>>>> Convert errx() and err() usage to appropriate TAP13 ksft API.
>>>>
>>>> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
>>>> ---
>>>>  tools/testing/selftests/capabilities/test_execve.c | 105 ++++++++++++---------
>>>>  .../testing/selftests/capabilities/validate_cap.c  |   9 +-
>>>>  2 files changed, 65 insertions(+), 49 deletions(-)
>>>>
>>>> diff --git a/tools/testing/selftests/capabilities/test_execve.c b/tools/testing/selftests/capabilities/test_execve.c
>>>> index 7c38233292b0..cf6778441381 100644
>>>> --- a/tools/testing/selftests/capabilities/test_execve.c
>>>> +++ b/tools/testing/selftests/capabilities/test_execve.c
>>>> @@ -1,7 +1,6 @@
>>>>  #define _GNU_SOURCE
>>>>
>>>>  #include <cap-ng.h>
>>>> -#include <err.h>
>>>>  #include <linux/capability.h>
>>>>  #include <stdbool.h>
>>>>  #include <string.h>
>>>> @@ -39,29 +38,32 @@ static void vmaybe_write_file(bool enoent_ok, char *filename, char *fmt, va_list
>>>>         int buf_len;
>>>>
>>>>         buf_len = vsnprintf(buf, sizeof(buf), fmt, ap);
>>>> -       if (buf_len < 0) {
>>>> -               err(1, "vsnprintf failed");
>>>> -       }
>>>> -       if (buf_len >= sizeof(buf)) {
>>>> -               errx(1, "vsnprintf output truncated");
>>>> -       }
>>>> +       if (buf_len < 0)
>>>> +               ksft_exit_fail_msg("vsnprintf failed - %s\n", strerror(errno));
>>>
>>> Could this not be a hypothetical ksft_exit_fail_msg_err or similar?
>>> Or a shorter name like ksft_fatal_err()?
>>>
>>>
>>
>> Is there a reason to add _err() suffix?
>>
>> ksft_exit_fail_msg() is a generic routine for a test to exit
>> with a test failure and print a message. The message doesn't
>> necessarily need to be a standard error message such as the
>> one err() or errx() or strerror() generate.
>>
>> In some cases test could fail with a standard error condition,
>> but not always. In that context, it doesn't make sense to add
>> _err suffix. I leveraged this generic function to replace err()
>> and errx() usages adding strerror() not loose the important
>> information.
> 
> The idea behind the _err version is to avoid the extra typing to
> report errno.  I suppose you could always report errno, but there are
> contexts where errno is meaningless or garbage.
> 
> 

Thinking about this some more, using strerror() as a replacements does
drop some information compared to what _err() and _errx() provide.

capabilities is the first test I came across that uses err() and errx()
heavily. I am sure there are more that do that. It might be useful to
provide a equivalent ksft_ framework replacement. I will work on it.

thanks,
-- Shuah

Re: [PATCH] selftests: capabilities: convert error output to TAP13 ksft framework

[Shuah Khan]

On 07/28/2017 09:41 AM, Shuah Khan wrote:
> On 07/27/2017 08:13 PM, Andy Lutomirski wrote:
>> On Thu, Jul 27, 2017 at 1:32 PM, Shuah Khan <shuah@kernel.org> wrote:
>>> On 07/27/2017 12:50 PM, Andy Lutomirski wrote:
>>>> On Wed, Jul 26, 2017 at 2:18 PM, Shuah Khan <shuahkh@osg.samsung.com> wrote:
>>>>> Convert errx() and err() usage to appropriate TAP13 ksft API.
>>>>>
>>>>> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
>>>>> ---
>>>>>  tools/testing/selftests/capabilities/test_execve.c | 105 ++++++++++++---------
>>>>>  .../testing/selftests/capabilities/validate_cap.c  |   9 +-
>>>>>  2 files changed, 65 insertions(+), 49 deletions(-)
>>>>>
>>>>> diff --git a/tools/testing/selftests/capabilities/test_execve.c b/tools/testing/selftests/capabilities/test_execve.c
>>>>> index 7c38233292b0..cf6778441381 100644
>>>>> --- a/tools/testing/selftests/capabilities/test_execve.c
>>>>> +++ b/tools/testing/selftests/capabilities/test_execve.c
>>>>> @@ -1,7 +1,6 @@
>>>>>  #define _GNU_SOURCE
>>>>>
>>>>>  #include <cap-ng.h>
>>>>> -#include <err.h>
>>>>>  #include <linux/capability.h>
>>>>>  #include <stdbool.h>
>>>>>  #include <string.h>
>>>>> @@ -39,29 +38,32 @@ static void vmaybe_write_file(bool enoent_ok, char *filename, char *fmt, va_list
>>>>>         int buf_len;
>>>>>
>>>>>         buf_len = vsnprintf(buf, sizeof(buf), fmt, ap);
>>>>> -       if (buf_len < 0) {
>>>>> -               err(1, "vsnprintf failed");
>>>>> -       }
>>>>> -       if (buf_len >= sizeof(buf)) {
>>>>> -               errx(1, "vsnprintf output truncated");
>>>>> -       }
>>>>> +       if (buf_len < 0)
>>>>> +               ksft_exit_fail_msg("vsnprintf failed - %s\n", strerror(errno));
>>>>
>>>> Could this not be a hypothetical ksft_exit_fail_msg_err or similar?
>>>> Or a shorter name like ksft_fatal_err()?
>>>>
>>>>
>>>
>>> Is there a reason to add _err() suffix?
>>>
>>> ksft_exit_fail_msg() is a generic routine for a test to exit
>>> with a test failure and print a message. The message doesn't
>>> necessarily need to be a standard error message such as the
>>> one err() or errx() or strerror() generate.
>>>
>>> In some cases test could fail with a standard error condition,
>>> but not always. In that context, it doesn't make sense to add
>>> _err suffix. I leveraged this generic function to replace err()
>>> and errx() usages adding strerror() not loose the important
>>> information.
>>
>> The idea behind the _err version is to avoid the extra typing to
>> report errno.  I suppose you could always report errno, but there are
>> contexts where errno is meaningless or garbage.
>>>>
> 
> Thinking about this some more, using strerror() as a replacements does
> drop some information compared to what _err() and _errx() provide.
> 
> capabilities is the first test I came across that uses err() and errx()
> heavily. I am sure there are more that do that. It might be useful to
> provide a equivalent ksft_ framework replacement. I will work on it.
> 

Okay. More on this. Both err() and errx() will not return. So, if we were
to preserve the usage either not replacing with hypothetical ksft_err() and
ksft_errx() will result in the error message being the last message of the
test output after the test results summary. Example:

Say the test ran into a failure that uses err() or errx() in the middle:

If err() and errx() aren't replaced:

Test will end without printing results summary:
"error message from err() or errx()"


If err() and errx() are replaced:

not ok 9 failed
Pass 9 Fail 1 Xfail 0 Xpass 0 Skip 0
1..9
"error message from err() or errx()"

Not replacing err() and errx() isn't good. Replacing addresses the
issue of printing counts, but test ends with an error message. This
isn't great from TAP13 compliance and it is lot cleaner to see:

"error message from err() or errx()"
not ok 9 failed
Pass 9 Fail 1 Xfail 0 Xpass 0 Skip 0
1..9

In the interest if presenting cleaner and easier to understand output,
we might have to opt for using strerror(errno) assuming that in the
cases when err() and errx() are in use, errno will be valid.

thanks,
-- Shuah

Re: [PATCH] selftests: capabilities: convert error output to TAP13 ksft framework

[Shuah Khan]

Hi Andy,

On 07/28/2017 03:39 PM, Shuah Khan wrote:
> On 07/28/2017 09:41 AM, Shuah Khan wrote:
>> On 07/27/2017 08:13 PM, Andy Lutomirski wrote:
>>> On Thu, Jul 27, 2017 at 1:32 PM, Shuah Khan <shuah@kernel.org> wrote:
>>>> On 07/27/2017 12:50 PM, Andy Lutomirski wrote:
>>>>> On Wed, Jul 26, 2017 at 2:18 PM, Shuah Khan <shuahkh@osg.samsung.com> wrote:
>>>>>> Convert errx() and err() usage to appropriate TAP13 ksft API.
>>>>>>
>>>>>> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
>>>>>> ---
>>>>>>  tools/testing/selftests/capabilities/test_execve.c | 105 ++++++++++++---------
>>>>>>  .../testing/selftests/capabilities/validate_cap.c  |   9 +-
>>>>>>  2 files changed, 65 insertions(+), 49 deletions(-)
>>>>>>
>>>>>> diff --git a/tools/testing/selftests/capabilities/test_execve.c b/tools/testing/selftests/capabilities/test_execve.c
>>>>>> index 7c38233292b0..cf6778441381 100644
>>>>>> --- a/tools/testing/selftests/capabilities/test_execve.c
>>>>>> +++ b/tools/testing/selftests/capabilities/test_execve.c
>>>>>> @@ -1,7 +1,6 @@
>>>>>>  #define _GNU_SOURCE
>>>>>>
>>>>>>  #include <cap-ng.h>
>>>>>> -#include <err.h>
>>>>>>  #include <linux/capability.h>
>>>>>>  #include <stdbool.h>
>>>>>>  #include <string.h>
>>>>>> @@ -39,29 +38,32 @@ static void vmaybe_write_file(bool enoent_ok, char *filename, char *fmt, va_list
>>>>>>         int buf_len;
>>>>>>
>>>>>>         buf_len = vsnprintf(buf, sizeof(buf), fmt, ap);
>>>>>> -       if (buf_len < 0) {
>>>>>> -               err(1, "vsnprintf failed");
>>>>>> -       }
>>>>>> -       if (buf_len >= sizeof(buf)) {
>>>>>> -               errx(1, "vsnprintf output truncated");
>>>>>> -       }
>>>>>> +       if (buf_len < 0)
>>>>>> +               ksft_exit_fail_msg("vsnprintf failed - %s\n", strerror(errno));
>>>>>
>>>>> Could this not be a hypothetical ksft_exit_fail_msg_err or similar?
>>>>> Or a shorter name like ksft_fatal_err()?
>>>>>
>>>>>
>>>>
>>>> Is there a reason to add _err() suffix?
>>>>
>>>> ksft_exit_fail_msg() is a generic routine for a test to exit
>>>> with a test failure and print a message. The message doesn't
>>>> necessarily need to be a standard error message such as the
>>>> one err() or errx() or strerror() generate.
>>>>
>>>> In some cases test could fail with a standard error condition,
>>>> but not always. In that context, it doesn't make sense to add
>>>> _err suffix. I leveraged this generic function to replace err()
>>>> and errx() usages adding strerror() not loose the important
>>>> information.
>>>
>>> The idea behind the _err version is to avoid the extra typing to
>>> report errno.  I suppose you could always report errno, but there are
>>> contexts where errno is meaningless or garbage.
>>>>>
>>
>> Thinking about this some more, using strerror() as a replacements does
>> drop some information compared to what _err() and _errx() provide.
>>
>> capabilities is the first test I came across that uses err() and errx()
>> heavily. I am sure there are more that do that. It might be useful to
>> provide a equivalent ksft_ framework replacement. I will work on it.
>>
> 
> Okay. More on this. Both err() and errx() will not return. So, if we were
> to preserve the usage either not replacing with hypothetical ksft_err() and
> ksft_errx() will result in the error message being the last message of the
> test output after the test results summary. Example:
> 
> Say the test ran into a failure that uses err() or errx() in the middle:
> 
> If err() and errx() aren't replaced:
> 
> Test will end without printing results summary:
> "error message from err() or errx()"
> 
> 
> If err() and errx() are replaced:
> 
> not ok 9 failed
> Pass 9 Fail 1 Xfail 0 Xpass 0 Skip 0
> 1..9
> "error message from err() or errx()"
> 
> Not replacing err() and errx() isn't good. Replacing addresses the
> issue of printing counts, but test ends with an error message. This
> isn't great from TAP13 compliance and it is lot cleaner to see:
> 
> "error message from err() or errx()"
> not ok 9 failed
> Pass 9 Fail 1 Xfail 0 Xpass 0 Skip 0
> 1..9
> 
> In the interest if presenting cleaner and easier to understand output,
> we might have to opt for using strerror(errno) assuming that in the
> cases when err() and errx() are in use, errno will be valid.
> 

Are you okay with this change? If you think it is very important to
have the information, there is an option to add ksft versions for
err() and errx() that do everything err() and errx() do except not
exit. There is a downside to this that we have to maintain these.

Please let me know your preference.

thanks,
-- Shuah