* [LTP] [PATCH 0/6] C API: .needs_cmds and SAFE_RUN_CMD()
@ 2020-03-27 21:39 Petr Vorel
2020-03-27 21:39 ` [LTP] [PATCH 1/6] lib: Implement .needs_cmds Petr Vorel
` (6 more replies)
0 siblings, 7 replies; 30+ messages in thread
From: Petr Vorel @ 2020-03-27 21:39 UTC (permalink / raw)
To: ltp
Hi,
I implemented .needs_cmds + SAFE_RUN_CMD().
I also checked for command in path in tst_run_cmd_fds_() as suggested by
Cyril + added possibility to TCONF when program not found. This leaded
to more verbose changes. + There is a double check (first in
.needs_cmds, then in SAFE_RUN_CMD()), maybe that's not needed.
Also this change is not used in copy_file_range02.c, because it does not
require these commands. Is it worth to reduce code it'd be good to separate
file into two, put common functions in header and tests with
fd_immutable and fd_swapfile into new test?
Petr Vorel (6):
lib: Implement .needs_cmds
Use .needs_cmds
lib/tst_run_cmd_*(): Turn int pass_exit_val into enum
lib/tst_run_cmd_*(): Search for program in $PATH
lib: Implement SAFE_RUN_CMD() macro (new API only)
Use SAFE_RUN_CMD()
doc/test-writing-guidelines.txt | 29 +++++++++++---
include/tst_cmd.h | 39 +++++++++++--------
include/tst_safe_macros.h | 20 ++++++++++
include/tst_test.h | 3 ++
lib/tst_kernel.c | 3 +-
lib/tst_mkfs.c | 3 +-
lib/tst_module.c | 3 +-
lib/tst_run_cmd.c | 28 ++++++++-----
lib/tst_test.c | 11 ++++++
testcases/kernel/input/input_helper.c | 4 +-
testcases/kernel/syscalls/acct/acct02.c | 2 +-
testcases/kernel/syscalls/add_key/add_key05.c | 24 +++++-------
.../copy_file_range/copy_file_range02.c | 2 +-
.../kernel/syscalls/quotactl/quotactl01.c | 18 +++------
.../kernel/syscalls/quotactl/quotactl06.c | 16 +++-----
.../syscalls/setpriority/setpriority01.c | 5 ++-
testcases/network/netstress/netstress.c | 2 +-
17 files changed, 134 insertions(+), 78 deletions(-)
--
2.25.1
^ permalink raw reply [flat|nested] 30+ messages in thread
* [LTP] [PATCH 1/6] lib: Implement .needs_cmds
2020-03-27 21:39 [LTP] [PATCH 0/6] C API: .needs_cmds and SAFE_RUN_CMD() Petr Vorel
@ 2020-03-27 21:39 ` Petr Vorel
2020-03-30 6:13 ` Li Wang
2020-03-27 21:39 ` [LTP] [PATCH 2/6] Use .needs_cmds Petr Vorel
` (5 subsequent siblings)
6 siblings, 1 reply; 30+ messages in thread
From: Petr Vorel @ 2020-03-27 21:39 UTC (permalink / raw)
To: ltp
Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
New commit.
doc/test-writing-guidelines.txt | 15 +++++++++++++++
include/tst_test.h | 3 +++
lib/tst_test.c | 11 +++++++++++
3 files changed, 29 insertions(+)
diff --git a/doc/test-writing-guidelines.txt b/doc/test-writing-guidelines.txt
index 32c9e87df..f7206f1bf 100644
--- a/doc/test-writing-guidelines.txt
+++ b/doc/test-writing-guidelines.txt
@@ -2023,6 +2023,21 @@ struct tst_test test = {
};
-------------------------------------------------------------------------------
+2.2.35 Checking for required binaries
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+Required binaries can be checked with '.needs_cmds', which points to a 'NULL'
+terminated array of strings such as:
+
+[source,c]
+-------------------------------------------------------------------------------
+.needs_cmds = (const char *const []) {
+ "useradd",
+ "userdel",
+ NULL
+},
+-------------------------------------------------------------------------------
+
2.3 Writing a testcase in shell
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
diff --git a/include/tst_test.h b/include/tst_test.h
index 84b6a940f..592097084 100644
--- a/include/tst_test.h
+++ b/include/tst_test.h
@@ -239,6 +239,9 @@ struct tst_test {
* {NULL, NULL} terminated array of tags.
*/
const struct tst_tag *tags;
+
+ /* NULL terminated array of required binaries */
+ const char *const *needs_cmds;
};
/*
diff --git a/lib/tst_test.c b/lib/tst_test.c
index 220d7fdfc..dae3fa1b5 100644
--- a/lib/tst_test.c
+++ b/lib/tst_test.c
@@ -3,6 +3,7 @@
* Copyright (c) 2015-2016 Cyril Hrubis <chrubis@suse.cz>
*/
+#include <limits.h>
#include <stdio.h>
#include <stdarg.h>
#include <unistd.h>
@@ -880,6 +881,16 @@ static void do_setup(int argc, char *argv[])
if (tst_test->min_kver)
check_kver();
+ if (tst_test->needs_cmds) {
+ const char *cmd;
+ char path[PATH_MAX];
+ int i;
+
+ for (i = 0; (cmd = tst_test->needs_cmds[i]); ++i)
+ if (tst_get_path(cmd, path, sizeof(path)))
+ tst_brk(TCONF, "Couldn't find '%s' in $PATH", cmd);
+ }
+
if (tst_test->needs_drivers) {
const char *name;
int i;
--
2.25.1
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [LTP] [PATCH 2/6] Use .needs_cmds
2020-03-27 21:39 [LTP] [PATCH 0/6] C API: .needs_cmds and SAFE_RUN_CMD() Petr Vorel
2020-03-27 21:39 ` [LTP] [PATCH 1/6] lib: Implement .needs_cmds Petr Vorel
@ 2020-03-27 21:39 ` Petr Vorel
2020-03-30 11:31 ` Cyril Hrubis
2020-03-27 21:39 ` [LTP] [PATCH 3/6] lib/tst_run_cmd_*(): Turn int pass_exit_val into enum Petr Vorel
` (4 subsequent siblings)
6 siblings, 1 reply; 30+ messages in thread
From: Petr Vorel @ 2020-03-27 21:39 UTC (permalink / raw)
To: ltp
Change for quotactl01: require also userdel
Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
New commit.
testcases/kernel/syscalls/add_key/add_key05.c | 8 +++++---
testcases/kernel/syscalls/quotactl/quotactl01.c | 7 ++++---
testcases/kernel/syscalls/quotactl/quotactl06.c | 7 ++++---
3 files changed, 13 insertions(+), 9 deletions(-)
diff --git a/testcases/kernel/syscalls/add_key/add_key05.c b/testcases/kernel/syscalls/add_key/add_key05.c
index a39bfa0b7..7443a4970 100644
--- a/testcases/kernel/syscalls/add_key/add_key05.c
+++ b/testcases/kernel/syscalls/add_key/add_key05.c
@@ -43,9 +43,6 @@ static void add_user(void)
user_added = 1;
ltpuser = SAFE_GETPWNAM(username);
break;
- case 255:
- tst_brk(TCONF, "useradd not found");
- break;
default:
tst_brk(TBROK, "useradd failed (%d)", rc);
}
@@ -215,6 +212,11 @@ static struct tst_test test = {
{&user_buf, .size = 64},
{}
},
+ .needs_cmds = (const char *const []) {
+ "useradd",
+ "userdel",
+ NULL
+ },
.tags = (const struct tst_tag[]) {
{"linux-git", "a08bf91ce28"},
{"linux-git", "2e356101e72"},
diff --git a/testcases/kernel/syscalls/quotactl/quotactl01.c b/testcases/kernel/syscalls/quotactl/quotactl01.c
index ede61d7e4..6cc1deeb8 100644
--- a/testcases/kernel/syscalls/quotactl/quotactl01.c
+++ b/testcases/kernel/syscalls/quotactl/quotactl01.c
@@ -168,9 +168,6 @@ static void setup(void)
switch (ret) {
case 0:
break;
- case 255:
- tst_brk(TCONF, "quotacheck binary not installed");
- break;
default:
tst_brk(TBROK, "quotacheck exited with %i", ret);
}
@@ -234,5 +231,9 @@ static struct tst_test test = {
.dev_fs_type = "ext4",
.mntpoint = MNTPOINT,
.mnt_data = "usrquota,grpquota",
+ .needs_cmds = (const char *const []) {
+ "quotacheck",
+ NULL
+ },
.setup = setup,
};
diff --git a/testcases/kernel/syscalls/quotactl/quotactl06.c b/testcases/kernel/syscalls/quotactl/quotactl06.c
index a3b4517e0..758bd84cd 100644
--- a/testcases/kernel/syscalls/quotactl/quotactl06.c
+++ b/testcases/kernel/syscalls/quotactl/quotactl06.c
@@ -153,9 +153,6 @@ static void setup(void)
switch (ret) {
case 0:
break;
- case 255:
- tst_brk(TCONF, "quotacheck binary not installed");
- break;
default:
tst_brk(TBROK, "quotacheck exited with %i", ret);
}
@@ -192,5 +189,9 @@ static struct tst_test test = {
.mntpoint = MNTPOINT,
.mount_device = 1,
.mnt_data = "usrquota",
+ .needs_cmds = (const char *const []) {
+ "quotacheck",
+ NULL
+ },
.needs_root = 1,
};
--
2.25.1
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [LTP] [PATCH 3/6] lib/tst_run_cmd_*(): Turn int pass_exit_val into enum
2020-03-27 21:39 [LTP] [PATCH 0/6] C API: .needs_cmds and SAFE_RUN_CMD() Petr Vorel
2020-03-27 21:39 ` [LTP] [PATCH 1/6] lib: Implement .needs_cmds Petr Vorel
2020-03-27 21:39 ` [LTP] [PATCH 2/6] Use .needs_cmds Petr Vorel
@ 2020-03-27 21:39 ` Petr Vorel
2020-03-30 11:38 ` Cyril Hrubis
2020-03-27 21:39 ` [LTP] [PATCH 4/6] lib/tst_run_cmd_*(): Search for program in $PATH Petr Vorel
` (3 subsequent siblings)
6 siblings, 1 reply; 30+ messages in thread
From: Petr Vorel @ 2020-03-27 21:39 UTC (permalink / raw)
To: ltp
This is a preparation for next commit.
Now uses the only flag TST_RUN_CMD_PASS_EXIT_VAL.
Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
New commit.
doc/test-writing-guidelines.txt | 10 +++---
include/tst_cmd.h | 36 ++++++++++---------
lib/tst_kernel.c | 3 +-
lib/tst_mkfs.c | 3 +-
lib/tst_module.c | 3 +-
lib/tst_run_cmd.c | 12 +++----
testcases/kernel/input/input_helper.c | 4 +--
testcases/kernel/syscalls/acct/acct02.c | 2 +-
testcases/kernel/syscalls/add_key/add_key05.c | 5 +--
.../copy_file_range/copy_file_range02.c | 2 +-
.../kernel/syscalls/quotactl/quotactl01.c | 2 +-
.../kernel/syscalls/quotactl/quotactl06.c | 2 +-
.../syscalls/setpriority/setpriority01.c | 5 +--
testcases/network/netstress/netstress.c | 2 +-
14 files changed, 50 insertions(+), 41 deletions(-)
diff --git a/doc/test-writing-guidelines.txt b/doc/test-writing-guidelines.txt
index f7206f1bf..31897309d 100644
--- a/doc/test-writing-guidelines.txt
+++ b/doc/test-writing-guidelines.txt
@@ -1263,18 +1263,18 @@ different once the call returns and should be used only for rough estimates.
int tst_run_cmd(const char *const argv[],
const char *stdout_path,
const char *stderr_path,
- int pass_exit_val);
+ enum tst_run_cmd_flags flags);
-------------------------------------------------------------------------------
-'tst_run_cmd' is a wrapper for 'vfork() + execvp()' which provides a way
+'tst_run_cmd()' is a wrapper for 'vfork() + execvp()' which provides a way
to execute an external program.
'argv[]' is a 'NULL' terminated array of strings starting with the program name
which is followed by optional arguments.
-A non-zero 'pass_exit_val' makes 'tst_run_cmd' return the program exit code to
-the caller. A zero for 'pass_exit_val' makes 'tst_run_cmd' exit the tests
-on failure.
+'TST_RUN_CMD_PASS_EXIT_VAL' enum 'tst_run_cmd_flags' makes 'tst_run_cmd()'
+return the program exit code to the caller, otherwise 'tst_run_cmd()' exit the
+tests on failure.
In case that 'execvp()' has failed and the 'pass_exit_val' flag was set, the
return value is '255' if 'execvp()' failed with 'ENOENT' and '254' otherwise.
diff --git a/include/tst_cmd.h b/include/tst_cmd.h
index d0a3573f4..67dec32f2 100644
--- a/include/tst_cmd.h
+++ b/include/tst_cmd.h
@@ -5,6 +5,14 @@
#ifndef TST_CMD_H__
#define TST_CMD_H__
+enum tst_run_cmd_flags {
+ /*
+ * return the program exit code, otherwise it will call cleanup_fn() if the
+ * program exit code is not zero.
+ */
+ TST_RUN_CMD_PASS_EXIT_VAL = 1,
+};
+
/*
* vfork() + execvp() specified program.
* @argv: a list of two (at least program name + NULL) or more pointers that
@@ -14,68 +22,64 @@
* redirection is not needed.
* @stderr_fd: file descriptor where to redirect stderr. Set -1 if
* redirection is not needed.
- * @pass_exit_val: if it's non-zero, this function will return the program
- * exit code, otherwise it will call cleanup_fn() if the program
- * exit code is not zero.
+ * @flags: enum tst_run_cmd_flags
*/
int tst_run_cmd_fds_(void (cleanup_fn)(void),
const char *const argv[],
int stdout_fd,
int stderr_fd,
- int pass_exit_val);
+ enum tst_run_cmd_flags flags);
/* Executes tst_run_cmd_fds() and redirects its output to a file
* @stdout_path: path where to redirect stdout. Set NULL if redirection is
* not needed.
* @stderr_path: path where to redirect stderr. Set NULL if redirection is
* not needed.
- * @pass_exit_val: if it's non-zero, this function will return the program
- * exit code, otherwise it will call cleanup_fn() if the program
- * exit code is not zero.
+ * @flags: enum tst_run_cmd_flags
*/
int tst_run_cmd_(void (cleanup_fn)(void),
const char *const argv[],
const char *stdout_path,
const char *stderr_path,
- int pass_exit_val);
+ enum tst_run_cmd_flags flags);
#ifdef TST_TEST_H__
static inline int tst_run_cmd_fds(const char *const argv[],
int stdout_fd,
int stderr_fd,
- int pass_exit_val)
+ enum tst_run_cmd_flags flags)
{
return tst_run_cmd_fds_(NULL, argv,
- stdout_fd, stderr_fd, pass_exit_val);
+ stdout_fd, stderr_fd, flags);
}
static inline int tst_run_cmd(const char *const argv[],
const char *stdout_path,
const char *stderr_path,
- int pass_exit_val)
+ enum tst_run_cmd_flags flags)
{
return tst_run_cmd_(NULL, argv,
- stdout_path, stderr_path, pass_exit_val);
+ stdout_path, stderr_path, flags);
}
#else
static inline int tst_run_cmd_fds(void (cleanup_fn)(void),
const char *const argv[],
int stdout_fd,
int stderr_fd,
- int pass_exit_val)
+ enum tst_run_cmd_flags flags)
{
return tst_run_cmd_fds_(cleanup_fn, argv,
- stdout_fd, stderr_fd, pass_exit_val);
+ stdout_fd, stderr_fd, flags);
}
static inline int tst_run_cmd(void (cleanup_fn)(void),
const char *const argv[],
const char *stdout_path,
const char *stderr_path,
- int pass_exit_val)
+ enum tst_run_cmd_flags flags)
{
return tst_run_cmd_(cleanup_fn, argv,
- stdout_path, stderr_path, pass_exit_val);
+ stdout_path, stderr_path, flags);
}
#endif
diff --git a/lib/tst_kernel.c b/lib/tst_kernel.c
index 73ab9f1b1..3ef7c21b8 100644
--- a/lib/tst_kernel.c
+++ b/lib/tst_kernel.c
@@ -85,7 +85,8 @@ int tst_check_driver(const char *name)
{
#ifndef __ANDROID__
const char * const argv[] = { "modprobe", "-n", name, NULL };
- int res = tst_run_cmd_(NULL, argv, "/dev/null", "/dev/null", 1);
+ int res = tst_run_cmd_(NULL, argv, "/dev/null", "/dev/null",
+ TST_RUN_CMD_PASS_EXIT_VAL);
/* 255 - it looks like modprobe not available */
return (res == 255) ? 0 : res;
diff --git a/lib/tst_mkfs.c b/lib/tst_mkfs.c
index 5d208eceb..0c347e1e2 100644
--- a/lib/tst_mkfs.c
+++ b/lib/tst_mkfs.c
@@ -88,7 +88,8 @@ void tst_mkfs_(const char *file, const int lineno, void (cleanup_fn)(void),
tst_resm(TINFO, "Formatting %s with %s opts='%s' extra opts='%s'",
dev, fs_type, fs_opts_str, extra_opts_str);
- ret = tst_run_cmd(cleanup_fn, argv, "/dev/null", NULL, 1);
+ ret = tst_run_cmd(cleanup_fn, argv, "/dev/null", NULL,
+ TST_RUN_CMD_PASS_EXIT_VAL);
switch (ret) {
case 0:
diff --git a/lib/tst_module.c b/lib/tst_module.c
index ed39952ee..9344b59d6 100644
--- a/lib/tst_module.c
+++ b/lib/tst_module.c
@@ -109,7 +109,8 @@ void tst_module_unload(void (cleanup_fn)(void), const char *mod_name)
rc = 1;
for (i = 0; i < 50; i++) {
- rc = tst_run_cmd(NULL, argv, "/dev/null", "/dev/null", 1);
+ rc = tst_run_cmd(NULL, argv, "/dev/null", "/dev/null",
+ TST_RUN_CMD_PASS_EXIT_VAL);
if (!rc)
break;
diff --git a/lib/tst_run_cmd.c b/lib/tst_run_cmd.c
index 8e4bf6ba3..3536ec494 100644
--- a/lib/tst_run_cmd.c
+++ b/lib/tst_run_cmd.c
@@ -1,5 +1,6 @@
/*
* Copyright (c) 2013 Oracle and/or its affiliates. All Rights Reserved.
+ * Copyright (c) 2020 Petr Vorel <pvorel@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
@@ -16,7 +17,6 @@
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
*
* Author: Alexey Kodanev <alexey.kodanev@oracle.com>
- *
*/
#include <errno.h>
@@ -27,6 +27,7 @@
#include <unistd.h>
#include <signal.h>
#include "test.h"
+#include "tst_cmd.h"
#define OPEN_MODE (S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH)
#define OPEN_FLAGS (O_WRONLY | O_APPEND | O_CREAT)
@@ -35,7 +36,7 @@ int tst_run_cmd_fds_(void (cleanup_fn)(void),
const char *const argv[],
int stdout_fd,
int stderr_fd,
- int pass_exit_val)
+ enum tst_run_cmd_flags flags)
{
int rc;
@@ -97,7 +98,7 @@ int tst_run_cmd_fds_(void (cleanup_fn)(void),
rc = WEXITSTATUS(ret);
- if ((!pass_exit_val) && rc) {
+ if (!(flags & TST_RUN_CMD_PASS_EXIT_VAL) && rc) {
tst_brkm(TBROK, cleanup_fn,
"'%s' exited with a non-zero code %d at %s:%d",
argv[0], rc, __FILE__, __LINE__);
@@ -111,7 +112,7 @@ int tst_run_cmd_(void (cleanup_fn)(void),
const char *const argv[],
const char *stdout_path,
const char *stderr_path,
- int pass_exit_val)
+ enum tst_run_cmd_flags flags)
{
int stdout_fd = -1;
int stderr_fd = -1;
@@ -137,8 +138,7 @@ int tst_run_cmd_(void (cleanup_fn)(void),
stderr_path, __FILE__, __LINE__);
}
- rc = tst_run_cmd_fds(cleanup_fn, argv, stdout_fd, stderr_fd,
- pass_exit_val);
+ rc = tst_run_cmd_fds(cleanup_fn, argv, stdout_fd, stderr_fd, flags);
if ((stdout_fd != -1) && (close(stdout_fd) == -1))
tst_resm(TWARN | TERRNO,
diff --git a/testcases/kernel/input/input_helper.c b/testcases/kernel/input/input_helper.c
index f6ae9c9b6..41c042aaf 100644
--- a/testcases/kernel/input/input_helper.c
+++ b/testcases/kernel/input/input_helper.c
@@ -92,7 +92,7 @@ static int try_load_uinput(void)
tst_resm(TINFO, "Trying to load uinput kernel module");
- ret = tst_run_cmd(NULL, argv, NULL, NULL, 1);
+ ret = tst_run_cmd(NULL, argv, NULL, NULL, TST_RUN_CMD_PASS_EXIT_VAL);
if (ret) {
tst_resm(TINFO, "Failed to load the uinput module");
return 0;
@@ -108,7 +108,7 @@ static void unload_uinput(void)
tst_resm(TINFO, "Unloading uinput kernel module");
- ret = tst_run_cmd(NULL, argv, NULL, NULL, 1);
+ ret = tst_run_cmd(NULL, argv, NULL, NULL, TST_RUN_CMD_PASS_EXIT_VAL);
if (ret)
tst_resm(TWARN, "Failed to unload uinput module");
}
diff --git a/testcases/kernel/syscalls/acct/acct02.c b/testcases/kernel/syscalls/acct/acct02.c
index 2f1290fa2..801c7908d 100644
--- a/testcases/kernel/syscalls/acct/acct02.c
+++ b/testcases/kernel/syscalls/acct/acct02.c
@@ -67,7 +67,7 @@ static void run_command(void)
{
const char *const cmd[] = {COMMAND, NULL};
- rc = tst_run_cmd(cmd, NULL, NULL, 1) << 8;
+ rc = tst_run_cmd(cmd, NULL, NULL, TST_RUN_CMD_PASS_EXIT_VAL) << 8;
}
static int verify_acct(void *acc, int elap_time)
diff --git a/testcases/kernel/syscalls/add_key/add_key05.c b/testcases/kernel/syscalls/add_key/add_key05.c
index 7443a4970..3da3be9ee 100644
--- a/testcases/kernel/syscalls/add_key/add_key05.c
+++ b/testcases/kernel/syscalls/add_key/add_key05.c
@@ -38,7 +38,8 @@ static void add_user(void)
const char *const cmd_useradd[] = {"useradd", username, NULL};
int rc;
- switch ((rc = tst_run_cmd(cmd_useradd, NULL, NULL, 1))) {
+ switch ((rc = tst_run_cmd(cmd_useradd, NULL, NULL,
+ TST_RUN_CMD_PASS_EXIT_VAL))) {
case 0:
user_added = 1;
ltpuser = SAFE_GETPWNAM(username);
@@ -56,7 +57,7 @@ static void clean_user(void)
const char *const cmd_userdel[] = {"userdel", "-r", username, NULL};
- if (tst_run_cmd(cmd_userdel, NULL, NULL, 1))
+ if (tst_run_cmd(cmd_userdel, NULL, NULL, TST_RUN_CMD_PASS_EXIT_VAL))
tst_res(TWARN | TERRNO, "'userdel -r %s' failed", username);
else
user_added = 0;
diff --git a/testcases/kernel/syscalls/copy_file_range/copy_file_range02.c b/testcases/kernel/syscalls/copy_file_range/copy_file_range02.c
index c09766fe6..f47479f16 100644
--- a/testcases/kernel/syscalls/copy_file_range/copy_file_range02.c
+++ b/testcases/kernel/syscalls/copy_file_range/copy_file_range02.c
@@ -87,7 +87,7 @@ static int run_command(char *command, char *option, char *file)
const char *const cmd[] = {command, option, file, NULL};
int ret;
- ret = tst_run_cmd(cmd, NULL, NULL, 1);
+ ret = tst_run_cmd(cmd, NULL, NULL, TST_RUN_CMD_PASS_EXIT_VAL);
switch (ret) {
case 0:
return 0;
diff --git a/testcases/kernel/syscalls/quotactl/quotactl01.c b/testcases/kernel/syscalls/quotactl/quotactl01.c
index 6cc1deeb8..e4c2a8939 100644
--- a/testcases/kernel/syscalls/quotactl/quotactl01.c
+++ b/testcases/kernel/syscalls/quotactl/quotactl01.c
@@ -164,7 +164,7 @@ static void setup(void)
const char *const cmd[] = {"quotacheck", "-ugF", "vfsv0", MNTPOINT, NULL};
int ret;
- ret = tst_run_cmd(cmd, NULL, NULL, 1);
+ ret = tst_run_cmd(cmd, NULL, NULL, TST_RUN_CMD_PASS_EXIT_VAL);
switch (ret) {
case 0:
break;
diff --git a/testcases/kernel/syscalls/quotactl/quotactl06.c b/testcases/kernel/syscalls/quotactl/quotactl06.c
index 758bd84cd..5d70f340f 100644
--- a/testcases/kernel/syscalls/quotactl/quotactl06.c
+++ b/testcases/kernel/syscalls/quotactl/quotactl06.c
@@ -149,7 +149,7 @@ static void setup(void)
int ret;
unsigned int i;
- ret = tst_run_cmd(cmd, NULL, NULL, 1);
+ ret = tst_run_cmd(cmd, NULL, NULL, TST_RUN_CMD_PASS_EXIT_VAL);
switch (ret) {
case 0:
break;
diff --git a/testcases/kernel/syscalls/setpriority/setpriority01.c b/testcases/kernel/syscalls/setpriority/setpriority01.c
index ffd7499c4..0c6689fd5 100644
--- a/testcases/kernel/syscalls/setpriority/setpriority01.c
+++ b/testcases/kernel/syscalls/setpriority/setpriority01.c
@@ -112,7 +112,8 @@ static void setup(void)
struct passwd *ltpuser;
int rc;
- switch ((rc = tst_run_cmd(cmd_useradd, NULL, NULL, 1))) {
+ switch ((rc = tst_run_cmd(cmd_useradd, NULL, NULL,
+ TST_RUN_CMD_PASS_EXIT_VAL))) {
case 0:
user_added = 1;
ltpuser = SAFE_GETPWNAM(username);
@@ -133,7 +134,7 @@ static void cleanup(void)
const char *const cmd_userdel[] = {"userdel", "-r", username, NULL};
- if (tst_run_cmd(cmd_userdel, NULL, NULL, 1))
+ if (tst_run_cmd(cmd_userdel, NULL, NULL, TST_RUN_CMD_PASS_EXIT_VAL))
tst_res(TWARN | TERRNO, "'userdel -r %s' failed", username);
}
diff --git a/testcases/network/netstress/netstress.c b/testcases/network/netstress/netstress.c
index 76d2fdb04..8d85ad085 100644
--- a/testcases/network/netstress/netstress.c
+++ b/testcases/network/netstress/netstress.c
@@ -967,7 +967,7 @@ static void setup(void)
/* dccp* modules can be blacklisted, load them manually */
const char * const argv[] = {"modprobe", "dccp_ipv6", NULL};
- if (tst_run_cmd(argv, NULL, NULL, 1))
+ if (tst_run_cmd(argv, NULL, NULL, TST_RUN_CMD_PASS_EXIT_VAL))
tst_brk(TCONF, "Failed to load dccp_ipv6 module");
tst_res(TINFO, "DCCP %s", (client_mode) ? "client" : "server");
--
2.25.1
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [LTP] [PATCH 4/6] lib/tst_run_cmd_*(): Search for program in $PATH
2020-03-27 21:39 [LTP] [PATCH 0/6] C API: .needs_cmds and SAFE_RUN_CMD() Petr Vorel
` (2 preceding siblings ...)
2020-03-27 21:39 ` [LTP] [PATCH 3/6] lib/tst_run_cmd_*(): Turn int pass_exit_val into enum Petr Vorel
@ 2020-03-27 21:39 ` Petr Vorel
2020-03-30 11:40 ` Cyril Hrubis
2020-03-27 21:39 ` [LTP] [PATCH 5/6] lib: Implement SAFE_RUN_CMD() macro (new API only) Petr Vorel
` (2 subsequent siblings)
6 siblings, 1 reply; 30+ messages in thread
From: Petr Vorel @ 2020-03-27 21:39 UTC (permalink / raw)
To: ltp
before calling execvp(). This is slightly safer than checking errno ENOENT.
TST_RUN_CMD_CHECK_CMD flag cause TBROK when program not found.
Suggested-by: Cyril Hrubis <chrubis@suse.cz>
Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
New commit.
doc/test-writing-guidelines.txt | 3 ++-
include/tst_cmd.h | 3 +++
lib/tst_run_cmd.c | 16 ++++++++++++----
3 files changed, 17 insertions(+), 5 deletions(-)
diff --git a/doc/test-writing-guidelines.txt b/doc/test-writing-guidelines.txt
index 31897309d..51eba6e39 100644
--- a/doc/test-writing-guidelines.txt
+++ b/doc/test-writing-guidelines.txt
@@ -1274,7 +1274,8 @@ which is followed by optional arguments.
'TST_RUN_CMD_PASS_EXIT_VAL' enum 'tst_run_cmd_flags' makes 'tst_run_cmd()'
return the program exit code to the caller, otherwise 'tst_run_cmd()' exit the
-tests on failure.
+tests on failure. 'TST_RUN_CMD_CHECK_CMD' check for program in '$PATH' and exit
+with 'TCONF' if not found.
In case that 'execvp()' has failed and the 'pass_exit_val' flag was set, the
return value is '255' if 'execvp()' failed with 'ENOENT' and '254' otherwise.
diff --git a/include/tst_cmd.h b/include/tst_cmd.h
index 67dec32f2..cab25462e 100644
--- a/include/tst_cmd.h
+++ b/include/tst_cmd.h
@@ -11,6 +11,9 @@ enum tst_run_cmd_flags {
* program exit code is not zero.
*/
TST_RUN_CMD_PASS_EXIT_VAL = 1,
+
+ /* exit with TCONF if program is not in path */
+ TST_RUN_CMD_CHECK_CMD = 2,
};
/*
diff --git a/lib/tst_run_cmd.c b/lib/tst_run_cmd.c
index 3536ec494..0494c6083 100644
--- a/lib/tst_run_cmd.c
+++ b/lib/tst_run_cmd.c
@@ -56,6 +56,17 @@ int tst_run_cmd_fds_(void (cleanup_fn)(void),
*/
void *old_handler = signal(SIGCHLD, SIG_DFL);
+ const char *cmd;
+ char path[PATH_MAX];
+
+ if (tst_get_path(argv[0], path, sizeof(path))) {
+ if (flags & TST_RUN_CMD_CHECK_CMD)
+ tst_brkm(TCONF, "Couldn't find '%s' in $PATH at %s:%d", argv[0],
+ __FILE__, __LINE__);
+ else
+ _exit(255);
+ }
+
pid_t pid = vfork();
if (pid == -1) {
tst_brkm(TBROK | TERRNO, cleanup_fn, "vfork failed at %s:%d",
@@ -74,10 +85,7 @@ int tst_run_cmd_fds_(void (cleanup_fn)(void),
dup2(stderr_fd, STDERR_FILENO);
}
- if (execvp(argv[0], (char *const *)argv)) {
- if (errno == ENOENT)
- _exit(255);
- }
+ execvp(argv[0], (char *const *)argv);
_exit(254);
}
--
2.25.1
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [LTP] [PATCH 5/6] lib: Implement SAFE_RUN_CMD() macro (new API only)
2020-03-27 21:39 [LTP] [PATCH 0/6] C API: .needs_cmds and SAFE_RUN_CMD() Petr Vorel
` (3 preceding siblings ...)
2020-03-27 21:39 ` [LTP] [PATCH 4/6] lib/tst_run_cmd_*(): Search for program in $PATH Petr Vorel
@ 2020-03-27 21:39 ` Petr Vorel
2020-03-30 6:35 ` Li Wang
2020-03-27 21:39 ` [LTP] [PATCH 6/6] Use SAFE_RUN_CMD() Petr Vorel
2020-03-28 2:41 ` [LTP] [PATCH 0/6] C API: .needs_cmds and SAFE_RUN_CMD() Xiao Yang
6 siblings, 1 reply; 30+ messages in thread
From: Petr Vorel @ 2020-03-27 21:39 UTC (permalink / raw)
To: ltp
Reviewed-by: Yang Xu <xuyang2018.jy@cn.fujitsu.com>
Reviewed-by: Li Wang <liwang@redhat.com>
Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
Renamed: s/SAFE_RUNCMD()/SAFE_RUN_CMD()/
doc/test-writing-guidelines.txt | 3 +++
include/tst_safe_macros.h | 20 ++++++++++++++++++++
2 files changed, 23 insertions(+)
diff --git a/doc/test-writing-guidelines.txt b/doc/test-writing-guidelines.txt
index 51eba6e39..4b195a002 100644
--- a/doc/test-writing-guidelines.txt
+++ b/doc/test-writing-guidelines.txt
@@ -1283,6 +1283,9 @@ return value is '255' if 'execvp()' failed with 'ENOENT' and '254' otherwise.
'stdout_path' and 'stderr_path' determine where to redirect the program
stdout and stderr I/O streams.
+The 'SAFE_RUN_CMD()' macro can be used automatic handling non zero exits (exits
+with 'TBROK') or 'ENOENT' (exits with 'TCONF').
+
.Example
[source,c]
-------------------------------------------------------------------------------
diff --git a/include/tst_safe_macros.h b/include/tst_safe_macros.h
index d95d26219..af45bc51d 100644
--- a/include/tst_safe_macros.h
+++ b/include/tst_safe_macros.h
@@ -21,6 +21,7 @@
#include <grp.h>
#include "safe_macros_fn.h"
+#include "tst_cmd.h"
#define SAFE_BASENAME(path) \
safe_basename(__FILE__, __LINE__, NULL, (path))
@@ -534,4 +535,23 @@ int safe_personality(const char *filename, unsigned int lineno,
void safe_unshare(const char *file, const int lineno, int flags);
#define SAFE_UNSHARE(flags) safe_unshare(__FILE__, __LINE__, (flags))
+static inline void safe_run_cmd(const char *file, const int lineno,
+ const char *const argv[],
+ const char *stdout_path,
+ const char *stderr_path)
+{
+ int rval;
+
+ switch ((rval = tst_run_cmd(argv, stdout_path, stderr_path,
+ TST_RUN_CMD_PASS_EXIT_VAL |
+ TST_RUN_CMD_CHECK_CMD))) {
+ case 0:
+ break;
+ default:
+ tst_brk(TBROK, "%s:%d: %s failed (%d)", file, lineno, argv[0], rval);
+ }
+}
+#define SAFE_RUN_CMD(argv, stdout_path, stderr_path) \
+ safe_run_cmd(__FILE__, __LINE__, (argv), (stdout_path), (stderr_path))
+
#endif /* SAFE_MACROS_H__ */
--
2.25.1
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [LTP] [PATCH 6/6] Use SAFE_RUN_CMD()
2020-03-27 21:39 [LTP] [PATCH 0/6] C API: .needs_cmds and SAFE_RUN_CMD() Petr Vorel
` (4 preceding siblings ...)
2020-03-27 21:39 ` [LTP] [PATCH 5/6] lib: Implement SAFE_RUN_CMD() macro (new API only) Petr Vorel
@ 2020-03-27 21:39 ` Petr Vorel
2020-03-28 2:41 ` [LTP] [PATCH 0/6] C API: .needs_cmds and SAFE_RUN_CMD() Xiao Yang
6 siblings, 0 replies; 30+ messages in thread
From: Petr Vorel @ 2020-03-27 21:39 UTC (permalink / raw)
To: ltp
Reviewed-by: Yang Xu <xuyang2018.jy@cn.fujitsu.com>
Reviewed-by: Li Wang <liwang@redhat.com>
Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
Renamed: s/SAFE_RUNCMD()/SAFE_RUN_CMD()/
testcases/kernel/syscalls/add_key/add_key05.c | 15 ++++-----------
testcases/kernel/syscalls/quotactl/quotactl01.c | 11 ++---------
testcases/kernel/syscalls/quotactl/quotactl06.c | 9 +--------
3 files changed, 7 insertions(+), 28 deletions(-)
diff --git a/testcases/kernel/syscalls/add_key/add_key05.c b/testcases/kernel/syscalls/add_key/add_key05.c
index 3da3be9ee..fd0fb0a50 100644
--- a/testcases/kernel/syscalls/add_key/add_key05.c
+++ b/testcases/kernel/syscalls/add_key/add_key05.c
@@ -36,17 +36,10 @@ static void add_user(void)
return;
const char *const cmd_useradd[] = {"useradd", username, NULL};
- int rc;
-
- switch ((rc = tst_run_cmd(cmd_useradd, NULL, NULL,
- TST_RUN_CMD_PASS_EXIT_VAL))) {
- case 0:
- user_added = 1;
- ltpuser = SAFE_GETPWNAM(username);
- break;
- default:
- tst_brk(TBROK, "useradd failed (%d)", rc);
- }
+
+ SAFE_RUN_CMD(cmd_useradd, NULL, NULL);
+ user_added = 1;
+ ltpuser = SAFE_GETPWNAM(username);
sprintf(fmt, "%5u: %%*5d %%*d/%%*d %%d/%%d %%d/%%d", ltpuser->pw_uid);
}
diff --git a/testcases/kernel/syscalls/quotactl/quotactl01.c b/testcases/kernel/syscalls/quotactl/quotactl01.c
index e4c2a8939..a40852f34 100644
--- a/testcases/kernel/syscalls/quotactl/quotactl01.c
+++ b/testcases/kernel/syscalls/quotactl/quotactl01.c
@@ -162,15 +162,8 @@ static struct tcase {
static void setup(void)
{
const char *const cmd[] = {"quotacheck", "-ugF", "vfsv0", MNTPOINT, NULL};
- int ret;
-
- ret = tst_run_cmd(cmd, NULL, NULL, TST_RUN_CMD_PASS_EXIT_VAL);
- switch (ret) {
- case 0:
- break;
- default:
- tst_brk(TBROK, "quotacheck exited with %i", ret);
- }
+
+ SAFE_RUN_CMD(cmd, NULL, NULL);
test_id = geteuid();
if (access(USRPATH, F_OK) == -1)
diff --git a/testcases/kernel/syscalls/quotactl/quotactl06.c b/testcases/kernel/syscalls/quotactl/quotactl06.c
index 5d70f340f..e990d6976 100644
--- a/testcases/kernel/syscalls/quotactl/quotactl06.c
+++ b/testcases/kernel/syscalls/quotactl/quotactl06.c
@@ -146,16 +146,9 @@ static void verify_quotactl(unsigned int n)
static void setup(void)
{
const char *const cmd[] = {"quotacheck", "-uF", "vfsv0", MNTPOINT, NULL};
- int ret;
unsigned int i;
- ret = tst_run_cmd(cmd, NULL, NULL, TST_RUN_CMD_PASS_EXIT_VAL);
- switch (ret) {
- case 0:
- break;
- default:
- tst_brk(TBROK, "quotacheck exited with %i", ret);
- }
+ SAFE_RUN_CMD(cmd, NULL, NULL);
if (access(USRPATH, F_OK) == -1)
tst_brk(TFAIL | TERRNO, "user quotafile didn't exist");
--
2.25.1
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [LTP] [PATCH 0/6] C API: .needs_cmds and SAFE_RUN_CMD()
2020-03-27 21:39 [LTP] [PATCH 0/6] C API: .needs_cmds and SAFE_RUN_CMD() Petr Vorel
` (5 preceding siblings ...)
2020-03-27 21:39 ` [LTP] [PATCH 6/6] Use SAFE_RUN_CMD() Petr Vorel
@ 2020-03-28 2:41 ` Xiao Yang
2020-03-28 3:42 ` Petr Vorel
6 siblings, 1 reply; 30+ messages in thread
From: Xiao Yang @ 2020-03-28 2:41 UTC (permalink / raw)
To: ltp
On 3/28/20 5:39 AM, Petr Vorel wrote:
> + There is a double check (first in
> .needs_cmds, then in SAFE_RUN_CMD()), maybe that's not needed.
Hi Petr,
Why do you need the duplicate .needs_cmds flag?(it introduces the double
check as you said)
Usually, all tests run commands by tst_run_cmd()/SAFE_RUN_CMD() and they
can report TCONF
by passing TST_RUN_CMD_CHECK_CMD so it is fair to be a part of
tst_run_cmd()/SAFE_RUN_CMD().
Thanks,
Xiao Yang
^ permalink raw reply [flat|nested] 30+ messages in thread
* [LTP] [PATCH 0/6] C API: .needs_cmds and SAFE_RUN_CMD()
2020-03-28 2:41 ` [LTP] [PATCH 0/6] C API: .needs_cmds and SAFE_RUN_CMD() Xiao Yang
@ 2020-03-28 3:42 ` Petr Vorel
2020-03-29 5:29 ` Xiao Yang
2020-03-30 4:24 ` Li Wang
0 siblings, 2 replies; 30+ messages in thread
From: Petr Vorel @ 2020-03-28 3:42 UTC (permalink / raw)
To: ltp
Hi Xiao,
> On 3/28/20 5:39 AM, Petr Vorel wrote:
> > + There is a double check (first in
> > .needs_cmds, then in SAFE_RUN_CMD()), maybe that's not needed.
> Hi Petr,
> Why do you need the duplicate .needs_cmds flag?(it introduces the double
> check as you said)
> Usually, all tests run commands by tst_run_cmd()/SAFE_RUN_CMD() and they can
> report TCONF
> by passing TST_RUN_CMD_CHECK_CMD so it is fair to be a part of
> tst_run_cmd()/SAFE_RUN_CMD().
Thanks for your review.
I guess Cyril will prefer .needs_cmds, as it can be parsed - metadata project:
https://people.kernel.org/metan/towards-parallel-kernel-test-runs
https://github.com/metan-ucw/ltp/tree/master/docparse
I put it there because some command might be run just under some condition (not
always), thus not suitable for .needs_cmds, but still nice to have reliable
check. But maybe I'm wrong.
Kind regards,
Petr
^ permalink raw reply [flat|nested] 30+ messages in thread
* [LTP] [PATCH 0/6] C API: .needs_cmds and SAFE_RUN_CMD()
2020-03-28 3:42 ` Petr Vorel
@ 2020-03-29 5:29 ` Xiao Yang
2020-03-30 4:39 ` Li Wang
2020-03-30 4:24 ` Li Wang
1 sibling, 1 reply; 30+ messages in thread
From: Xiao Yang @ 2020-03-29 5:29 UTC (permalink / raw)
To: ltp
On 2020/3/28 11:42, Petr Vorel wrote:
> Hi Xiao,
>
>> On 3/28/20 5:39 AM, Petr Vorel wrote:
>>> + There is a double check (first in
>>> .needs_cmds, then in SAFE_RUN_CMD()), maybe that's not needed.
>
>> Hi Petr,
>
>> Why do you need the duplicate .needs_cmds flag?(it introduces the double
>> check as you said)
>
>> Usually, all tests run commands by tst_run_cmd()/SAFE_RUN_CMD() and they can
>> report TCONF
>
>> by passing TST_RUN_CMD_CHECK_CMD so it is fair to be a part of
>> tst_run_cmd()/SAFE_RUN_CMD().
>
> Thanks for your review.
> I guess Cyril will prefer .needs_cmds, as it can be parsed - metadata project:
> https://people.kernel.org/metan/towards-parallel-kernel-test-runs
> https://github.com/metan-ucw/ltp/tree/master/docparse
Hi Petr?
Thank you for sharing these info.
Does Cyril want to get metadata from struct tst_test directly?
How about the rough design?
1) .needs_cmds only saves the required commands.(doesn't do any check)
2) pass the corresponding member of .needs_cmds to
tst_run_cmd()/SAFE_RUN_CMD()(do check in these functions).
For example:
----------------------------------------------
# grep tst_needs_cmds include/tst_cmd.h
extern const char *const *tst_needs_cmds;
# grep -B1 tst_needs_cmds lib/tst_test.c
const char *const *tst_needs_cmds;
--
if (tst_test->needs_cmds)
tst_needs_cmds = tst_test->needs_cmds;
# grep -A2 'needs_cmds' testcases/kernel/syscalls/add_key/add_key05.c
const char *const cmd_useradd[] = {tst_needs_cmds[0], username, NULL};
int rc;
--
const char *const cmd_userdel[] = {tst_needs_cmds[1], "-r",
username, NULL};
if (tst_run_cmd(cmd_userdel, NULL, NULL, 1))
--
.needs_cmds = (const char *const []) {
"useradd",
"userdel",
----------------------------------------------
btw: is it possible for .needs_cmds to save the required commands with
options?
Thanks,
Xiao Yang
>
> I put it there because some command might be run just under some condition (not
> always), thus not suitable for .needs_cmds, but still nice to have reliable
> check. But maybe I'm wrong.
>
> Kind regards,
> Petr
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* [LTP] [PATCH 0/6] C API: .needs_cmds and SAFE_RUN_CMD()
2020-03-28 3:42 ` Petr Vorel
2020-03-29 5:29 ` Xiao Yang
@ 2020-03-30 4:24 ` Li Wang
1 sibling, 0 replies; 30+ messages in thread
From: Li Wang @ 2020-03-30 4:24 UTC (permalink / raw)
To: ltp
On Sat, Mar 28, 2020 at 11:43 AM Petr Vorel <pvorel@suse.cz> wrote:
> Hi Xiao,
>
> > On 3/28/20 5:39 AM, Petr Vorel wrote:
> > > + There is a double check (first in
> > > .needs_cmds, then in SAFE_RUN_CMD()), maybe that's not needed.
>
> > Hi Petr,
>
> > Why do you need the duplicate .needs_cmds flag?(it introduces the double
> > check as you said)
>
> > Usually, all tests run commands by tst_run_cmd()/SAFE_RUN_CMD() and they
> can
> > report TCONF
>
> > by passing TST_RUN_CMD_CHECK_CMD so it is fair to be a part of
> > tst_run_cmd()/SAFE_RUN_CMD().
>
> Thanks for your review.
> I guess Cyril will prefer .needs_cmds, as it can be parsed - metadata
> project:
> https://people.kernel.org/metan/towards-parallel-kernel-test-runs
> https://github.com/metan-ucw/ltp/tree/master/docparse
>
> I put it there because some command might be run just under some condition
> (not
> always), thus not suitable for .needs_cmds, but still nice to have reliable
> check. But maybe I'm wrong.
>
+1 you're right, I believe that is safer and choosable for the test.
--
Regards,
Li Wang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20200330/e2d2298a/attachment.htm>
^ permalink raw reply [flat|nested] 30+ messages in thread
* [LTP] [PATCH 0/6] C API: .needs_cmds and SAFE_RUN_CMD()
2020-03-29 5:29 ` Xiao Yang
@ 2020-03-30 4:39 ` Li Wang
2020-03-30 5:20 ` Xiao Yang
0 siblings, 1 reply; 30+ messages in thread
From: Li Wang @ 2020-03-30 4:39 UTC (permalink / raw)
To: ltp
Hi Xiao,
On Sun, Mar 29, 2020 at 1:36 PM Xiao Yang <yangx.jy@cn.fujitsu.com> wrote:
> On 2020/3/28 11:42, Petr Vorel wrote:
> > Hi Xiao,
> >
> >> On 3/28/20 5:39 AM, Petr Vorel wrote:
> >>> + There is a double check (first in
> >>> .needs_cmds, then in SAFE_RUN_CMD()), maybe that's not needed.
> >
> >> Hi Petr,
> >
> >> Why do you need the duplicate .needs_cmds flag?(it introduces the double
> >> check as you said)
> >
> >> Usually, all tests run commands by tst_run_cmd()/SAFE_RUN_CMD() and
> they can
> >> report TCONF
> >
> >> by passing TST_RUN_CMD_CHECK_CMD so it is fair to be a part of
> >> tst_run_cmd()/SAFE_RUN_CMD().
> >
> > Thanks for your review.
> > I guess Cyril will prefer .needs_cmds, as it can be parsed - metadata
> project:
> > https://people.kernel.org/metan/towards-parallel-kernel-test-runs
> > https://github.com/metan-ucw/ltp/tree/master/docparse
> Hi Petr?
>
> Thank you for sharing these info.
> Does Cyril want to get metadata from struct tst_test directly?
>
> How about the rough design?
> 1) .needs_cmds only saves the required commands.(doesn't do any check)
> 2) pass the corresponding member of .needs_cmds to
> tst_run_cmd()/SAFE_RUN_CMD()(do check in these functions).
> For example:
> ----------------------------------------------
> # grep tst_needs_cmds include/tst_cmd.h
> extern const char *const *tst_needs_cmds;
>
> # grep -B1 tst_needs_cmds lib/tst_test.c
>
> const char *const *tst_needs_cmds;
> --
> if (tst_test->needs_cmds)
> tst_needs_cmds = tst_test->needs_cmds;
>
> # grep -A2 'needs_cmds' testcases/kernel/syscalls/add_key/add_key05.c
> const char *const cmd_useradd[] = {tst_needs_cmds[0], username, NULL};
> int rc;
>
I don't see any advantage of involving this struct in a test case, and it
also makes things more complicated.
IMO, the '.needs_cmds' should do check and guarantee all the
cmds exist. That's a hard requirement for the test. If a situation that the
commands are only part of the requirement(soft), we could avoid using
'.needs_cmds' in the test and just calling tst_run_cmd() without passing
TST_RUN_CMD_CHECK_CMD flag. This satisfies most situations we have, it is
safe enough and choosable for people.
Or maybe I'm wrong here too:).
--
Regards,
Li Wang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20200330/e959385c/attachment-0001.htm>
^ permalink raw reply [flat|nested] 30+ messages in thread
* [LTP] [PATCH 0/6] C API: .needs_cmds and SAFE_RUN_CMD()
2020-03-30 4:39 ` Li Wang
@ 2020-03-30 5:20 ` Xiao Yang
2020-03-30 7:12 ` Petr Vorel
0 siblings, 1 reply; 30+ messages in thread
From: Xiao Yang @ 2020-03-30 5:20 UTC (permalink / raw)
To: ltp
On 2020/3/30 12:39, Li Wang wrote:
> Hi Xiao,
>
> On Sun, Mar 29, 2020 at 1:36 PM Xiao Yang <yangx.jy@cn.fujitsu.com
> <mailto:yangx.jy@cn.fujitsu.com>> wrote:
>
> On 2020/3/28 11:42, Petr Vorel wrote:
> > Hi Xiao,
> >
> >> On 3/28/20 5:39 AM, Petr Vorel wrote:
> >>> + There is a double check (first in
> >>> .needs_cmds, then in SAFE_RUN_CMD()), maybe that's not needed.
> >
> >> Hi Petr,
> >
> >> Why do you need the duplicate .needs_cmds flag?(it introduces
> the double
> >> check as you said)
> >
> >> Usually, all tests run commands by tst_run_cmd()/SAFE_RUN_CMD()
> and they can
> >> report TCONF
> >
> >> by passing TST_RUN_CMD_CHECK_CMD so it is fair to be a part of
> >> tst_run_cmd()/SAFE_RUN_CMD().
> >
> > Thanks for your review.
> > I guess Cyril will prefer .needs_cmds, as it can be parsed -
> metadata project:
> > https://people.kernel.org/metan/towards-parallel-kernel-test-runs
> > https://github.com/metan-ucw/ltp/tree/master/docparse
> Hi Petr?
>
> Thank you for sharing these info.
> Does Cyril want to get metadata from struct tst_test directly?
>
> How about the rough design?
> 1) .needs_cmds only saves the required commands.(doesn't do any check)
> 2) pass the corresponding member of .needs_cmds to
> tst_run_cmd()/SAFE_RUN_CMD()(do check in these functions).
> For example:
> ----------------------------------------------
> # grep tst_needs_cmds include/tst_cmd.h
> extern const char *const *tst_needs_cmds;
>
> # grep -B1 tst_needs_cmds lib/tst_test.c
>
> const char *const *tst_needs_cmds;
> --
> if (tst_test->needs_cmds)
> tst_needs_cmds = tst_test->needs_cmds;
>
> # grep -A2 'needs_cmds' testcases/kernel/syscalls/add_key/add_key05.c
> const char *const cmd_useradd[] = {tst_needs_cmds[0], username, NULL};
> int rc;
>
>
> I don't see any advantage of involving this struct in a test case, and
> it also makes things more complicated.
Hi Li,
In fact, I perfer to remove .need_cmd and use tst_run_cmd with/without
TST_RUN_CMD_CHECK_CMD directly.
But I am not sure if it is necessary to keep .need_cmd for metadata
project. I think we can generate json about resouce by reading struct
tst_test or other ways.
Thanks,
Xiao Yang
>
> IMO, the '.needs_cmds' should do check and guarantee all the cmds exist.
> That's a hard requirement for the test. If a situation that the commands
> are only part of the requirement(soft), we could avoid using
> '.needs_cmds' in the test and just calling tst_run_cmd() without passing
> TST_RUN_CMD_CHECK_CMD flag. This satisfies most situations we have, it
> is safe enough and choosable for people.
>
> Or maybe I'm wrong here too:).
>
> --
> Regards,
> Li Wang
^ permalink raw reply [flat|nested] 30+ messages in thread
* [LTP] [PATCH 1/6] lib: Implement .needs_cmds
2020-03-27 21:39 ` [LTP] [PATCH 1/6] lib: Implement .needs_cmds Petr Vorel
@ 2020-03-30 6:13 ` Li Wang
2020-03-30 7:03 ` Petr Vorel
0 siblings, 1 reply; 30+ messages in thread
From: Li Wang @ 2020-03-30 6:13 UTC (permalink / raw)
To: ltp
On Sat, Mar 28, 2020 at 5:39 AM Petr Vorel <pvorel@suse.cz> wrote:
> Signed-off-by: Petr Vorel <pvorel@suse.cz>
> ---
> New commit.
>
> doc/test-writing-guidelines.txt | 15 +++++++++++++++
> include/tst_test.h | 3 +++
> lib/tst_test.c | 11 +++++++++++
> 3 files changed, 29 insertions(+)
>
> diff --git a/doc/test-writing-guidelines.txt
> b/doc/test-writing-guidelines.txt
> index 32c9e87df..f7206f1bf 100644
> --- a/doc/test-writing-guidelines.txt
> +++ b/doc/test-writing-guidelines.txt
> @@ -2023,6 +2023,21 @@ struct tst_test test = {
> };
>
> -------------------------------------------------------------------------------
>
> +2.2.35 Checking for required binaries
> +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
Maybe better to talk commands but not binaries, since not all of the
commands are binary, there is possible python, perl or shell executable
file need check too. Isn't it?
--
Regards,
Li Wang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20200330/eb0ca710/attachment.htm>
^ permalink raw reply [flat|nested] 30+ messages in thread
* [LTP] [PATCH 5/6] lib: Implement SAFE_RUN_CMD() macro (new API only)
2020-03-27 21:39 ` [LTP] [PATCH 5/6] lib: Implement SAFE_RUN_CMD() macro (new API only) Petr Vorel
@ 2020-03-30 6:35 ` Li Wang
2020-03-30 8:44 ` Petr Vorel
0 siblings, 1 reply; 30+ messages in thread
From: Li Wang @ 2020-03-30 6:35 UTC (permalink / raw)
To: ltp
Hi Petr,
Thanks for your work on this.
>
> +static inline void safe_run_cmd(const char *file, const int lineno,
> + const char
> *const argv[],
>
Be cautious of the code indent here, otherwise patchset looks good.
> + const char *stdout_path,
> + const char *stderr_path)
> +{
>
--
Regards,
Li Wang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20200330/0fcdfb80/attachment.htm>
^ permalink raw reply [flat|nested] 30+ messages in thread
* [LTP] [PATCH 1/6] lib: Implement .needs_cmds
2020-03-30 6:13 ` Li Wang
@ 2020-03-30 7:03 ` Petr Vorel
2020-03-30 11:31 ` Cyril Hrubis
0 siblings, 1 reply; 30+ messages in thread
From: Petr Vorel @ 2020-03-30 7:03 UTC (permalink / raw)
To: ltp
Hi Li,
thanks for your review.
> > +2.2.35 Checking for required binaries
> > +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> Maybe better to talk commands but not binaries, since not all of the
> commands are binary, there is possible python, perl or shell executable
> file need check too. Isn't it?
+1
Kind regards,
Petr
^ permalink raw reply [flat|nested] 30+ messages in thread
* [LTP] [PATCH 0/6] C API: .needs_cmds and SAFE_RUN_CMD()
2020-03-30 5:20 ` Xiao Yang
@ 2020-03-30 7:12 ` Petr Vorel
2020-03-30 7:43 ` Xiao Yang
0 siblings, 1 reply; 30+ messages in thread
From: Petr Vorel @ 2020-03-30 7:12 UTC (permalink / raw)
To: ltp
Hi Xiao,
> > # grep -A2 'needs_cmds' testcases/kernel/syscalls/add_key/add_key05.c
> > const char *const cmd_useradd[] = {tst_needs_cmds[0], username, NULL};
> > int rc;
> > I don't see any advantage of involving this struct in a test case, and
> > it also makes things more complicated.
> Hi Li,
> In fact, I perfer to remove .need_cmd and use tst_run_cmd with/without
> TST_RUN_CMD_CHECK_CMD directly.
> But I am not sure if it is necessary to keep .need_cmd for metadata project.
> I think we can generate json about resouce by reading struct tst_test or
> other ways.
not sure if you mean removing .needs_cmds entirely or just for
copy_file_range02.c. or some other test. I rewritten the original patchset
because Cyril suggested .needs_cmds implementation:
"Actually I would like to avoid exposing the function to the tests and
force people to use the .needs_cmds instead in order to have a proper
metadata." [1]
IMHO parsing struct members is easier to get metadata than searching for
various functions to be used, so I understand Cyril's intention. Cyril explains
this on his blog posts (I've noticed [2], but it's also in [3]: "this
arrangement also helps to export the test metadata into a machine parsable
format").
Kind regards,
Petr
> Thanks,
> Xiao Yang
> > IMO, the '.needs_cmds' should do check and guarantee all the cmds exist.
> > That's a hard requirement for the test. If a situation that the commands
> > are only part of the requirement(soft), we could avoid using
> > '.needs_cmds' in the test and just calling tst_run_cmd() without passing
> > TST_RUN_CMD_CHECK_CMD flag. This satisfies most situations we have, it
> > is safe enough and choosable for people.
> > Or maybe I'm wrong here too:).
> > --
> > Regards,
> > Li Wang
Kind regards,
Petr
[1] https://lists.linux.it/pipermail/ltp/2020-March/016233.html
[2] https://people.kernel.org/metan/towards-parallel-kernel-test-runs
[3] https://people.kernel.org/metan/the-ltp-test-driver-model
^ permalink raw reply [flat|nested] 30+ messages in thread
* [LTP] [PATCH 0/6] C API: .needs_cmds and SAFE_RUN_CMD()
2020-03-30 7:12 ` Petr Vorel
@ 2020-03-30 7:43 ` Xiao Yang
0 siblings, 0 replies; 30+ messages in thread
From: Xiao Yang @ 2020-03-30 7:43 UTC (permalink / raw)
To: ltp
On 2020/3/30 15:12, Petr Vorel wrote:
> Hi Xiao,
>
>>> # grep -A2 'needs_cmds' testcases/kernel/syscalls/add_key/add_key05.c
>>> const char *const cmd_useradd[] = {tst_needs_cmds[0], username, NULL};
>>> int rc;
>
>>> I don't see any advantage of involving this struct in a test case, and
>>> it also makes things more complicated.
>> Hi Li,
>
>> In fact, I perfer to remove .need_cmd and use tst_run_cmd with/without
>> TST_RUN_CMD_CHECK_CMD directly.
>> But I am not sure if it is necessary to keep .need_cmd for metadata project.
>> I think we can generate json about resouce by reading struct tst_test or
>> other ways.
>
> not sure if you mean removing .needs_cmds entirely or just for
> copy_file_range02.c. or some other test. I rewritten the original patchset
> because Cyril suggested .needs_cmds implementation:
>
> "Actually I would like to avoid exposing the function to the tests and
> force people to use the .needs_cmds instead in order to have a proper
> metadata." [1]
>
>
Hi Petr,
Thanks a lot for your explanation.
I want to remove .needs_cmds entirely before but it may be helpful to
get metadata about command.
Thanks,
Xiao Yang
> IMHO parsing struct members is easier to get metadata than searching for
> various functions to be used, so I understand Cyril's intention. Cyril explains
> this on his blog posts (I've noticed [2], but it's also in [3]: "this
> arrangement also helps to export the test metadata into a machine parsable
> format").
>
> Kind regards,
> Petr
>
>> Thanks,
>> Xiao Yang
>
>
>>> IMO, the '.needs_cmds' should do check and guarantee all the cmds exist.
>>> That's a hard requirement for the test. If a situation that the commands
>>> are only part of the requirement(soft), we could avoid using
>>> '.needs_cmds' in the test and just calling tst_run_cmd() without passing
>>> TST_RUN_CMD_CHECK_CMD flag. This satisfies most situations we have, it
>>> is safe enough and choosable for people.
>
>>> Or maybe I'm wrong here too:).
>
>>> --
>>> Regards,
>>> Li Wang
>
>
> Kind regards,
> Petr
>
> [1] https://lists.linux.it/pipermail/ltp/2020-March/016233.html
> [2] https://people.kernel.org/metan/towards-parallel-kernel-test-runs
> [3] https://people.kernel.org/metan/the-ltp-test-driver-model
>
>
> .
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* [LTP] [PATCH 5/6] lib: Implement SAFE_RUN_CMD() macro (new API only)
2020-03-30 6:35 ` Li Wang
@ 2020-03-30 8:44 ` Petr Vorel
0 siblings, 0 replies; 30+ messages in thread
From: Petr Vorel @ 2020-03-30 8:44 UTC (permalink / raw)
To: ltp
Hi Li,
> Thanks for your work on this.
> > +static inline void safe_run_cmd(const char *file, const int lineno,
Old code. 'static inline' shouldn't be here.
> > + const char
> > *const argv[],
> Be cautious of the code indent here, otherwise patchset looks good.
Thanks!
> > + const char *stdout_path,
> > + const char *stderr_path)
> > +{
Kind regards,
Petr
^ permalink raw reply [flat|nested] 30+ messages in thread
* [LTP] [PATCH 1/6] lib: Implement .needs_cmds
2020-03-30 7:03 ` Petr Vorel
@ 2020-03-30 11:31 ` Cyril Hrubis
0 siblings, 0 replies; 30+ messages in thread
From: Cyril Hrubis @ 2020-03-30 11:31 UTC (permalink / raw)
To: ltp
Hi!
> > > +2.2.35 Checking for required binaries
> > > +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> > Maybe better to talk commands but not binaries, since not all of the
> > commands are binary, there is possible python, perl or shell executable
> > file need check too. Isn't it?
> +1
Acked with this change.
--
Cyril Hrubis
chrubis@suse.cz
^ permalink raw reply [flat|nested] 30+ messages in thread
* [LTP] [PATCH 2/6] Use .needs_cmds
2020-03-27 21:39 ` [LTP] [PATCH 2/6] Use .needs_cmds Petr Vorel
@ 2020-03-30 11:31 ` Cyril Hrubis
2020-03-30 11:48 ` Petr Vorel
0 siblings, 1 reply; 30+ messages in thread
From: Cyril Hrubis @ 2020-03-30 11:31 UTC (permalink / raw)
To: ltp
Hi!
Looks good, acked.
--
Cyril Hrubis
chrubis@suse.cz
^ permalink raw reply [flat|nested] 30+ messages in thread
* [LTP] [PATCH 3/6] lib/tst_run_cmd_*(): Turn int pass_exit_val into enum
2020-03-27 21:39 ` [LTP] [PATCH 3/6] lib/tst_run_cmd_*(): Turn int pass_exit_val into enum Petr Vorel
@ 2020-03-30 11:38 ` Cyril Hrubis
2020-03-30 11:40 ` Petr Vorel
0 siblings, 1 reply; 30+ messages in thread
From: Cyril Hrubis @ 2020-03-30 11:38 UTC (permalink / raw)
To: ltp
Hi!
> This is a preparation for next commit.
> Now uses the only flag TST_RUN_CMD_PASS_EXIT_VAL.
That is quite long name, I wonder if we can shorten it.
Maybe we can rename the whole tst_run_cmd() just to tst_cmd() and change
this flag to TST_CMD_PASS_RETVAL.
Other that this the patchset looks good.
> Signed-off-by: Petr Vorel <pvorel@suse.cz>
> ---
> New commit.
>
> doc/test-writing-guidelines.txt | 10 +++---
> include/tst_cmd.h | 36 ++++++++++---------
> lib/tst_kernel.c | 3 +-
> lib/tst_mkfs.c | 3 +-
> lib/tst_module.c | 3 +-
> lib/tst_run_cmd.c | 12 +++----
> testcases/kernel/input/input_helper.c | 4 +--
> testcases/kernel/syscalls/acct/acct02.c | 2 +-
> testcases/kernel/syscalls/add_key/add_key05.c | 5 +--
> .../copy_file_range/copy_file_range02.c | 2 +-
> .../kernel/syscalls/quotactl/quotactl01.c | 2 +-
> .../kernel/syscalls/quotactl/quotactl06.c | 2 +-
> .../syscalls/setpriority/setpriority01.c | 5 +--
> testcases/network/netstress/netstress.c | 2 +-
> 14 files changed, 50 insertions(+), 41 deletions(-)
>
> diff --git a/doc/test-writing-guidelines.txt b/doc/test-writing-guidelines.txt
> index f7206f1bf..31897309d 100644
> --- a/doc/test-writing-guidelines.txt
> +++ b/doc/test-writing-guidelines.txt
> @@ -1263,18 +1263,18 @@ different once the call returns and should be used only for rough estimates.
> int tst_run_cmd(const char *const argv[],
> const char *stdout_path,
> const char *stderr_path,
> - int pass_exit_val);
> + enum tst_run_cmd_flags flags);
> -------------------------------------------------------------------------------
>
> -'tst_run_cmd' is a wrapper for 'vfork() + execvp()' which provides a way
> +'tst_run_cmd()' is a wrapper for 'vfork() + execvp()' which provides a way
> to execute an external program.
>
> 'argv[]' is a 'NULL' terminated array of strings starting with the program name
> which is followed by optional arguments.
>
> -A non-zero 'pass_exit_val' makes 'tst_run_cmd' return the program exit code to
> -the caller. A zero for 'pass_exit_val' makes 'tst_run_cmd' exit the tests
> -on failure.
> +'TST_RUN_CMD_PASS_EXIT_VAL' enum 'tst_run_cmd_flags' makes 'tst_run_cmd()'
> +return the program exit code to the caller, otherwise 'tst_run_cmd()' exit the
> +tests on failure.
>
> In case that 'execvp()' has failed and the 'pass_exit_val' flag was set, the
> return value is '255' if 'execvp()' failed with 'ENOENT' and '254' otherwise.
> diff --git a/include/tst_cmd.h b/include/tst_cmd.h
> index d0a3573f4..67dec32f2 100644
> --- a/include/tst_cmd.h
> +++ b/include/tst_cmd.h
> @@ -5,6 +5,14 @@
> #ifndef TST_CMD_H__
> #define TST_CMD_H__
>
> +enum tst_run_cmd_flags {
> + /*
> + * return the program exit code, otherwise it will call cleanup_fn() if the
> + * program exit code is not zero.
> + */
> + TST_RUN_CMD_PASS_EXIT_VAL = 1,
> +};
> +
> /*
> * vfork() + execvp() specified program.
> * @argv: a list of two (at least program name + NULL) or more pointers that
> @@ -14,68 +22,64 @@
> * redirection is not needed.
> * @stderr_fd: file descriptor where to redirect stderr. Set -1 if
> * redirection is not needed.
> - * @pass_exit_val: if it's non-zero, this function will return the program
> - * exit code, otherwise it will call cleanup_fn() if the program
> - * exit code is not zero.
> + * @flags: enum tst_run_cmd_flags
> */
> int tst_run_cmd_fds_(void (cleanup_fn)(void),
> const char *const argv[],
> int stdout_fd,
> int stderr_fd,
> - int pass_exit_val);
> + enum tst_run_cmd_flags flags);
>
> /* Executes tst_run_cmd_fds() and redirects its output to a file
> * @stdout_path: path where to redirect stdout. Set NULL if redirection is
> * not needed.
> * @stderr_path: path where to redirect stderr. Set NULL if redirection is
> * not needed.
> - * @pass_exit_val: if it's non-zero, this function will return the program
> - * exit code, otherwise it will call cleanup_fn() if the program
> - * exit code is not zero.
> + * @flags: enum tst_run_cmd_flags
> */
> int tst_run_cmd_(void (cleanup_fn)(void),
> const char *const argv[],
> const char *stdout_path,
> const char *stderr_path,
> - int pass_exit_val);
> + enum tst_run_cmd_flags flags);
>
> #ifdef TST_TEST_H__
> static inline int tst_run_cmd_fds(const char *const argv[],
> int stdout_fd,
> int stderr_fd,
> - int pass_exit_val)
> + enum tst_run_cmd_flags flags)
> {
> return tst_run_cmd_fds_(NULL, argv,
> - stdout_fd, stderr_fd, pass_exit_val);
> + stdout_fd, stderr_fd, flags);
> }
>
> static inline int tst_run_cmd(const char *const argv[],
> const char *stdout_path,
> const char *stderr_path,
> - int pass_exit_val)
> + enum tst_run_cmd_flags flags)
> {
> return tst_run_cmd_(NULL, argv,
> - stdout_path, stderr_path, pass_exit_val);
> + stdout_path, stderr_path, flags);
> }
> #else
> static inline int tst_run_cmd_fds(void (cleanup_fn)(void),
> const char *const argv[],
> int stdout_fd,
> int stderr_fd,
> - int pass_exit_val)
> + enum tst_run_cmd_flags flags)
> {
> return tst_run_cmd_fds_(cleanup_fn, argv,
> - stdout_fd, stderr_fd, pass_exit_val);
> + stdout_fd, stderr_fd, flags);
> }
>
> static inline int tst_run_cmd(void (cleanup_fn)(void),
> const char *const argv[],
> const char *stdout_path,
> const char *stderr_path,
> - int pass_exit_val)
> + enum tst_run_cmd_flags flags)
> {
> return tst_run_cmd_(cleanup_fn, argv,
> - stdout_path, stderr_path, pass_exit_val);
> + stdout_path, stderr_path, flags);
> }
> #endif
>
> diff --git a/lib/tst_kernel.c b/lib/tst_kernel.c
> index 73ab9f1b1..3ef7c21b8 100644
> --- a/lib/tst_kernel.c
> +++ b/lib/tst_kernel.c
> @@ -85,7 +85,8 @@ int tst_check_driver(const char *name)
> {
> #ifndef __ANDROID__
> const char * const argv[] = { "modprobe", "-n", name, NULL };
> - int res = tst_run_cmd_(NULL, argv, "/dev/null", "/dev/null", 1);
> + int res = tst_run_cmd_(NULL, argv, "/dev/null", "/dev/null",
> + TST_RUN_CMD_PASS_EXIT_VAL);
>
> /* 255 - it looks like modprobe not available */
> return (res == 255) ? 0 : res;
> diff --git a/lib/tst_mkfs.c b/lib/tst_mkfs.c
> index 5d208eceb..0c347e1e2 100644
> --- a/lib/tst_mkfs.c
> +++ b/lib/tst_mkfs.c
> @@ -88,7 +88,8 @@ void tst_mkfs_(const char *file, const int lineno, void (cleanup_fn)(void),
>
> tst_resm(TINFO, "Formatting %s with %s opts='%s' extra opts='%s'",
> dev, fs_type, fs_opts_str, extra_opts_str);
> - ret = tst_run_cmd(cleanup_fn, argv, "/dev/null", NULL, 1);
> + ret = tst_run_cmd(cleanup_fn, argv, "/dev/null", NULL,
> + TST_RUN_CMD_PASS_EXIT_VAL);
>
> switch (ret) {
> case 0:
> diff --git a/lib/tst_module.c b/lib/tst_module.c
> index ed39952ee..9344b59d6 100644
> --- a/lib/tst_module.c
> +++ b/lib/tst_module.c
> @@ -109,7 +109,8 @@ void tst_module_unload(void (cleanup_fn)(void), const char *mod_name)
>
> rc = 1;
> for (i = 0; i < 50; i++) {
> - rc = tst_run_cmd(NULL, argv, "/dev/null", "/dev/null", 1);
> + rc = tst_run_cmd(NULL, argv, "/dev/null", "/dev/null",
> + TST_RUN_CMD_PASS_EXIT_VAL);
> if (!rc)
> break;
>
> diff --git a/lib/tst_run_cmd.c b/lib/tst_run_cmd.c
> index 8e4bf6ba3..3536ec494 100644
> --- a/lib/tst_run_cmd.c
> +++ b/lib/tst_run_cmd.c
> @@ -1,5 +1,6 @@
> /*
> * Copyright (c) 2013 Oracle and/or its affiliates. All Rights Reserved.
> + * Copyright (c) 2020 Petr Vorel <pvorel@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
> @@ -16,7 +17,6 @@
> * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
> *
> * Author: Alexey Kodanev <alexey.kodanev@oracle.com>
> - *
> */
>
> #include <errno.h>
> @@ -27,6 +27,7 @@
> #include <unistd.h>
> #include <signal.h>
> #include "test.h"
> +#include "tst_cmd.h"
>
> #define OPEN_MODE (S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH)
> #define OPEN_FLAGS (O_WRONLY | O_APPEND | O_CREAT)
> @@ -35,7 +36,7 @@ int tst_run_cmd_fds_(void (cleanup_fn)(void),
> const char *const argv[],
> int stdout_fd,
> int stderr_fd,
> - int pass_exit_val)
> + enum tst_run_cmd_flags flags)
> {
> int rc;
>
> @@ -97,7 +98,7 @@ int tst_run_cmd_fds_(void (cleanup_fn)(void),
>
> rc = WEXITSTATUS(ret);
>
> - if ((!pass_exit_val) && rc) {
> + if (!(flags & TST_RUN_CMD_PASS_EXIT_VAL) && rc) {
> tst_brkm(TBROK, cleanup_fn,
> "'%s' exited with a non-zero code %d at %s:%d",
> argv[0], rc, __FILE__, __LINE__);
> @@ -111,7 +112,7 @@ int tst_run_cmd_(void (cleanup_fn)(void),
> const char *const argv[],
> const char *stdout_path,
> const char *stderr_path,
> - int pass_exit_val)
> + enum tst_run_cmd_flags flags)
> {
> int stdout_fd = -1;
> int stderr_fd = -1;
> @@ -137,8 +138,7 @@ int tst_run_cmd_(void (cleanup_fn)(void),
> stderr_path, __FILE__, __LINE__);
> }
>
> - rc = tst_run_cmd_fds(cleanup_fn, argv, stdout_fd, stderr_fd,
> - pass_exit_val);
> + rc = tst_run_cmd_fds(cleanup_fn, argv, stdout_fd, stderr_fd, flags);
>
> if ((stdout_fd != -1) && (close(stdout_fd) == -1))
> tst_resm(TWARN | TERRNO,
> diff --git a/testcases/kernel/input/input_helper.c b/testcases/kernel/input/input_helper.c
> index f6ae9c9b6..41c042aaf 100644
> --- a/testcases/kernel/input/input_helper.c
> +++ b/testcases/kernel/input/input_helper.c
> @@ -92,7 +92,7 @@ static int try_load_uinput(void)
>
> tst_resm(TINFO, "Trying to load uinput kernel module");
>
> - ret = tst_run_cmd(NULL, argv, NULL, NULL, 1);
> + ret = tst_run_cmd(NULL, argv, NULL, NULL, TST_RUN_CMD_PASS_EXIT_VAL);
> if (ret) {
> tst_resm(TINFO, "Failed to load the uinput module");
> return 0;
> @@ -108,7 +108,7 @@ static void unload_uinput(void)
>
> tst_resm(TINFO, "Unloading uinput kernel module");
>
> - ret = tst_run_cmd(NULL, argv, NULL, NULL, 1);
> + ret = tst_run_cmd(NULL, argv, NULL, NULL, TST_RUN_CMD_PASS_EXIT_VAL);
> if (ret)
> tst_resm(TWARN, "Failed to unload uinput module");
> }
> diff --git a/testcases/kernel/syscalls/acct/acct02.c b/testcases/kernel/syscalls/acct/acct02.c
> index 2f1290fa2..801c7908d 100644
> --- a/testcases/kernel/syscalls/acct/acct02.c
> +++ b/testcases/kernel/syscalls/acct/acct02.c
> @@ -67,7 +67,7 @@ static void run_command(void)
> {
> const char *const cmd[] = {COMMAND, NULL};
>
> - rc = tst_run_cmd(cmd, NULL, NULL, 1) << 8;
> + rc = tst_run_cmd(cmd, NULL, NULL, TST_RUN_CMD_PASS_EXIT_VAL) << 8;
> }
>
> static int verify_acct(void *acc, int elap_time)
> diff --git a/testcases/kernel/syscalls/add_key/add_key05.c b/testcases/kernel/syscalls/add_key/add_key05.c
> index 7443a4970..3da3be9ee 100644
> --- a/testcases/kernel/syscalls/add_key/add_key05.c
> +++ b/testcases/kernel/syscalls/add_key/add_key05.c
> @@ -38,7 +38,8 @@ static void add_user(void)
> const char *const cmd_useradd[] = {"useradd", username, NULL};
> int rc;
>
> - switch ((rc = tst_run_cmd(cmd_useradd, NULL, NULL, 1))) {
> + switch ((rc = tst_run_cmd(cmd_useradd, NULL, NULL,
> + TST_RUN_CMD_PASS_EXIT_VAL))) {
> case 0:
> user_added = 1;
> ltpuser = SAFE_GETPWNAM(username);
> @@ -56,7 +57,7 @@ static void clean_user(void)
>
> const char *const cmd_userdel[] = {"userdel", "-r", username, NULL};
>
> - if (tst_run_cmd(cmd_userdel, NULL, NULL, 1))
> + if (tst_run_cmd(cmd_userdel, NULL, NULL, TST_RUN_CMD_PASS_EXIT_VAL))
> tst_res(TWARN | TERRNO, "'userdel -r %s' failed", username);
> else
> user_added = 0;
> diff --git a/testcases/kernel/syscalls/copy_file_range/copy_file_range02.c b/testcases/kernel/syscalls/copy_file_range/copy_file_range02.c
> index c09766fe6..f47479f16 100644
> --- a/testcases/kernel/syscalls/copy_file_range/copy_file_range02.c
> +++ b/testcases/kernel/syscalls/copy_file_range/copy_file_range02.c
> @@ -87,7 +87,7 @@ static int run_command(char *command, char *option, char *file)
> const char *const cmd[] = {command, option, file, NULL};
> int ret;
>
> - ret = tst_run_cmd(cmd, NULL, NULL, 1);
> + ret = tst_run_cmd(cmd, NULL, NULL, TST_RUN_CMD_PASS_EXIT_VAL);
> switch (ret) {
> case 0:
> return 0;
> diff --git a/testcases/kernel/syscalls/quotactl/quotactl01.c b/testcases/kernel/syscalls/quotactl/quotactl01.c
> index 6cc1deeb8..e4c2a8939 100644
> --- a/testcases/kernel/syscalls/quotactl/quotactl01.c
> +++ b/testcases/kernel/syscalls/quotactl/quotactl01.c
> @@ -164,7 +164,7 @@ static void setup(void)
> const char *const cmd[] = {"quotacheck", "-ugF", "vfsv0", MNTPOINT, NULL};
> int ret;
>
> - ret = tst_run_cmd(cmd, NULL, NULL, 1);
> + ret = tst_run_cmd(cmd, NULL, NULL, TST_RUN_CMD_PASS_EXIT_VAL);
> switch (ret) {
> case 0:
> break;
> diff --git a/testcases/kernel/syscalls/quotactl/quotactl06.c b/testcases/kernel/syscalls/quotactl/quotactl06.c
> index 758bd84cd..5d70f340f 100644
> --- a/testcases/kernel/syscalls/quotactl/quotactl06.c
> +++ b/testcases/kernel/syscalls/quotactl/quotactl06.c
> @@ -149,7 +149,7 @@ static void setup(void)
> int ret;
> unsigned int i;
>
> - ret = tst_run_cmd(cmd, NULL, NULL, 1);
> + ret = tst_run_cmd(cmd, NULL, NULL, TST_RUN_CMD_PASS_EXIT_VAL);
> switch (ret) {
> case 0:
> break;
> diff --git a/testcases/kernel/syscalls/setpriority/setpriority01.c b/testcases/kernel/syscalls/setpriority/setpriority01.c
> index ffd7499c4..0c6689fd5 100644
> --- a/testcases/kernel/syscalls/setpriority/setpriority01.c
> +++ b/testcases/kernel/syscalls/setpriority/setpriority01.c
> @@ -112,7 +112,8 @@ static void setup(void)
> struct passwd *ltpuser;
> int rc;
>
> - switch ((rc = tst_run_cmd(cmd_useradd, NULL, NULL, 1))) {
> + switch ((rc = tst_run_cmd(cmd_useradd, NULL, NULL,
> + TST_RUN_CMD_PASS_EXIT_VAL))) {
> case 0:
> user_added = 1;
> ltpuser = SAFE_GETPWNAM(username);
> @@ -133,7 +134,7 @@ static void cleanup(void)
>
> const char *const cmd_userdel[] = {"userdel", "-r", username, NULL};
>
> - if (tst_run_cmd(cmd_userdel, NULL, NULL, 1))
> + if (tst_run_cmd(cmd_userdel, NULL, NULL, TST_RUN_CMD_PASS_EXIT_VAL))
> tst_res(TWARN | TERRNO, "'userdel -r %s' failed", username);
> }
>
> diff --git a/testcases/network/netstress/netstress.c b/testcases/network/netstress/netstress.c
> index 76d2fdb04..8d85ad085 100644
> --- a/testcases/network/netstress/netstress.c
> +++ b/testcases/network/netstress/netstress.c
> @@ -967,7 +967,7 @@ static void setup(void)
> /* dccp* modules can be blacklisted, load them manually */
> const char * const argv[] = {"modprobe", "dccp_ipv6", NULL};
>
> - if (tst_run_cmd(argv, NULL, NULL, 1))
> + if (tst_run_cmd(argv, NULL, NULL, TST_RUN_CMD_PASS_EXIT_VAL))
> tst_brk(TCONF, "Failed to load dccp_ipv6 module");
>
> tst_res(TINFO, "DCCP %s", (client_mode) ? "client" : "server");
> --
> 2.25.1
>
--
Cyril Hrubis
chrubis@suse.cz
^ permalink raw reply [flat|nested] 30+ messages in thread
* [LTP] [PATCH 4/6] lib/tst_run_cmd_*(): Search for program in $PATH
2020-03-27 21:39 ` [LTP] [PATCH 4/6] lib/tst_run_cmd_*(): Search for program in $PATH Petr Vorel
@ 2020-03-30 11:40 ` Cyril Hrubis
2020-03-30 11:52 ` Petr Vorel
0 siblings, 1 reply; 30+ messages in thread
From: Cyril Hrubis @ 2020-03-30 11:40 UTC (permalink / raw)
To: ltp
Hi!
> + /* exit with TCONF if program is not in path */
> + TST_RUN_CMD_CHECK_CMD = 2,
Shouldn't be this rather called TST_CMD_TCONF_ON_MISSING?
> };
>
> /*
> diff --git a/lib/tst_run_cmd.c b/lib/tst_run_cmd.c
> index 3536ec494..0494c6083 100644
> --- a/lib/tst_run_cmd.c
> +++ b/lib/tst_run_cmd.c
> @@ -56,6 +56,17 @@ int tst_run_cmd_fds_(void (cleanup_fn)(void),
> */
> void *old_handler = signal(SIGCHLD, SIG_DFL);
>
> + const char *cmd;
> + char path[PATH_MAX];
> +
> + if (tst_get_path(argv[0], path, sizeof(path))) {
> + if (flags & TST_RUN_CMD_CHECK_CMD)
> + tst_brkm(TCONF, "Couldn't find '%s' in $PATH at %s:%d", argv[0],
> + __FILE__, __LINE__);
> + else
> + _exit(255);
> + }
> +
> pid_t pid = vfork();
> if (pid == -1) {
> tst_brkm(TBROK | TERRNO, cleanup_fn, "vfork failed at %s:%d",
> @@ -74,10 +85,7 @@ int tst_run_cmd_fds_(void (cleanup_fn)(void),
> dup2(stderr_fd, STDERR_FILENO);
> }
>
> - if (execvp(argv[0], (char *const *)argv)) {
> - if (errno == ENOENT)
> - _exit(255);
> - }
> + execvp(argv[0], (char *const *)argv);
> _exit(254);
> }
>
> --
> 2.25.1
>
--
Cyril Hrubis
chrubis@suse.cz
^ permalink raw reply [flat|nested] 30+ messages in thread
* [LTP] [PATCH 3/6] lib/tst_run_cmd_*(): Turn int pass_exit_val into enum
2020-03-30 11:38 ` Cyril Hrubis
@ 2020-03-30 11:40 ` Petr Vorel
0 siblings, 0 replies; 30+ messages in thread
From: Petr Vorel @ 2020-03-30 11:40 UTC (permalink / raw)
To: ltp
Hi,
> > This is a preparation for next commit.
> > Now uses the only flag TST_RUN_CMD_PASS_EXIT_VAL.
> That is quite long name, I wonder if we can shorten it.
> Maybe we can rename the whole tst_run_cmd() just to tst_cmd() and change
> this flag to TST_CMD_PASS_RETVAL.
This looks good to me, I'll change it.
> Other that this the patchset looks good.
And push whole patchset with your, Li's and Xiao Reviewed-by: tag.
Kind regards,
Petr
^ permalink raw reply [flat|nested] 30+ messages in thread
* [LTP] [PATCH 2/6] Use .needs_cmds
2020-03-30 11:31 ` Cyril Hrubis
@ 2020-03-30 11:48 ` Petr Vorel
2020-03-30 11:57 ` Li Wang
0 siblings, 1 reply; 30+ messages in thread
From: Petr Vorel @ 2020-03-30 11:48 UTC (permalink / raw)
To: ltp
Hi,
> Hi!
> Looks good, acked.
thanks a lot for review.
Merged these two patches.
Kind regards,
Petr
^ permalink raw reply [flat|nested] 30+ messages in thread
* [LTP] [PATCH 4/6] lib/tst_run_cmd_*(): Search for program in $PATH
2020-03-30 11:40 ` Cyril Hrubis
@ 2020-03-30 11:52 ` Petr Vorel
2020-03-30 11:53 ` Cyril Hrubis
0 siblings, 1 reply; 30+ messages in thread
From: Petr Vorel @ 2020-03-30 11:52 UTC (permalink / raw)
To: ltp
Hi,
> > + /* exit with TCONF if program is not in path */
> > + TST_RUN_CMD_CHECK_CMD = 2,
> Shouldn't be this rather called TST_CMD_TCONF_ON_MISSING?
+1.
If these two comments (with renaming to tst_cmd() + flags) are the only your
concern, I'll rename it and just push without sending to ML.
When we're in renaming, I guess I should rename SAFE_RUN_CMD() into SAFE_CMD(),
ok?
Kind regards,
Petr
^ permalink raw reply [flat|nested] 30+ messages in thread
* [LTP] [PATCH 4/6] lib/tst_run_cmd_*(): Search for program in $PATH
2020-03-30 11:52 ` Petr Vorel
@ 2020-03-30 11:53 ` Cyril Hrubis
0 siblings, 0 replies; 30+ messages in thread
From: Cyril Hrubis @ 2020-03-30 11:53 UTC (permalink / raw)
To: ltp
Hi!
> > > + /* exit with TCONF if program is not in path */
> > > + TST_RUN_CMD_CHECK_CMD = 2,
>
> > Shouldn't be this rather called TST_CMD_TCONF_ON_MISSING?
> +1.
>
> If these two comments (with renaming to tst_cmd() + flags) are the only your
> concern, I'll rename it and just push without sending to ML.
Yes, minus the comments on the function and constant names the code
lookgs good.
> When we're in renaming, I guess I should rename SAFE_RUN_CMD() into SAFE_CMD(),
Indeed.
--
Cyril Hrubis
chrubis@suse.cz
^ permalink raw reply [flat|nested] 30+ messages in thread
* [LTP] [PATCH 2/6] Use .needs_cmds
2020-03-30 11:48 ` Petr Vorel
@ 2020-03-30 11:57 ` Li Wang
2020-03-30 12:17 ` Petr Vorel
2020-03-30 12:37 ` Petr Vorel
0 siblings, 2 replies; 30+ messages in thread
From: Li Wang @ 2020-03-30 11:57 UTC (permalink / raw)
To: ltp
On Mon, Mar 30, 2020 at 7:48 PM Petr Vorel <pvorel@suse.cz> wrote:
> Hi,
>
> > Hi!
> > Looks good, acked.
> thanks a lot for review.
>
> Merged these two patches.
>
Thanks Petr, there is still a tiny place need fix.
Hope you can help to correct the description in the next round patch
merging.
+ /* NULL terminated array of required binaries */
+ const char *const *needs_cmds;
--
Regards,
Li Wang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20200330/1489ca88/attachment.htm>
^ permalink raw reply [flat|nested] 30+ messages in thread
* [LTP] [PATCH 2/6] Use .needs_cmds
2020-03-30 11:57 ` Li Wang
@ 2020-03-30 12:17 ` Petr Vorel
2020-03-30 12:37 ` Petr Vorel
1 sibling, 0 replies; 30+ messages in thread
From: Petr Vorel @ 2020-03-30 12:17 UTC (permalink / raw)
To: ltp
Hi Li,
> > > Hi!
> > > Looks good, acked.
> > thanks a lot for review.
> > Merged these two patches.
> Thanks Petr, there is still a tiny place need fix.
> Hope you can help to correct the description in the next round patch
> merging.
Sure, thanks for catching it!
> + /* NULL terminated array of required binaries */
> + const char *const *needs_cmds;
Kind regards,
Petr
^ permalink raw reply [flat|nested] 30+ messages in thread
* [LTP] [PATCH 2/6] Use .needs_cmds
2020-03-30 11:57 ` Li Wang
2020-03-30 12:17 ` Petr Vorel
@ 2020-03-30 12:37 ` Petr Vorel
1 sibling, 0 replies; 30+ messages in thread
From: Petr Vorel @ 2020-03-30 12:37 UTC (permalink / raw)
To: ltp
Hi Li,
> Thanks Petr, there is still a tiny place need fix.
> Hope you can help to correct the description in the next round patch
> merging.
You were right, I pushed this before. I'm sorry, fixed now.
> + /* NULL terminated array of required binaries */
> + const char *const *needs_cmds;
Kind regards,
Petr
^ permalink raw reply [flat|nested] 30+ messages in thread
end of thread, other threads:[~2020-03-30 12:37 UTC | newest]
Thread overview: 30+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-27 21:39 [LTP] [PATCH 0/6] C API: .needs_cmds and SAFE_RUN_CMD() Petr Vorel
2020-03-27 21:39 ` [LTP] [PATCH 1/6] lib: Implement .needs_cmds Petr Vorel
2020-03-30 6:13 ` Li Wang
2020-03-30 7:03 ` Petr Vorel
2020-03-30 11:31 ` Cyril Hrubis
2020-03-27 21:39 ` [LTP] [PATCH 2/6] Use .needs_cmds Petr Vorel
2020-03-30 11:31 ` Cyril Hrubis
2020-03-30 11:48 ` Petr Vorel
2020-03-30 11:57 ` Li Wang
2020-03-30 12:17 ` Petr Vorel
2020-03-30 12:37 ` Petr Vorel
2020-03-27 21:39 ` [LTP] [PATCH 3/6] lib/tst_run_cmd_*(): Turn int pass_exit_val into enum Petr Vorel
2020-03-30 11:38 ` Cyril Hrubis
2020-03-30 11:40 ` Petr Vorel
2020-03-27 21:39 ` [LTP] [PATCH 4/6] lib/tst_run_cmd_*(): Search for program in $PATH Petr Vorel
2020-03-30 11:40 ` Cyril Hrubis
2020-03-30 11:52 ` Petr Vorel
2020-03-30 11:53 ` Cyril Hrubis
2020-03-27 21:39 ` [LTP] [PATCH 5/6] lib: Implement SAFE_RUN_CMD() macro (new API only) Petr Vorel
2020-03-30 6:35 ` Li Wang
2020-03-30 8:44 ` Petr Vorel
2020-03-27 21:39 ` [LTP] [PATCH 6/6] Use SAFE_RUN_CMD() Petr Vorel
2020-03-28 2:41 ` [LTP] [PATCH 0/6] C API: .needs_cmds and SAFE_RUN_CMD() Xiao Yang
2020-03-28 3:42 ` Petr Vorel
2020-03-29 5:29 ` Xiao Yang
2020-03-30 4:39 ` Li Wang
2020-03-30 5:20 ` Xiao Yang
2020-03-30 7:12 ` Petr Vorel
2020-03-30 7:43 ` Xiao Yang
2020-03-30 4:24 ` Li Wang
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.