[LTP] [PATCH 1/2] lib: Implement SAFE_RUN() macro (new API only)

[LTP] [PATCH 1/2] lib: Implement SAFE_RUN() macro (new API only)

[Petr Vorel]

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
 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 b56f1a0f2..b8330801b 100644
--- a/doc/test-writing-guidelines.txt
+++ b/doc/test-writing-guidelines.txt
@@ -1282,6 +1282,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_RUNCMD()' 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 80c4d9cb9..d701a003f 100644
--- a/include/tst_safe_macros.h
+++ b/include/tst_safe_macros.h
@@ -547,4 +547,24 @@ 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, 1))) {
+	case 0:
+		break;
+	case 255:
+		tst_brk(TCONF, "%s:%d: %s not found in $PATH", file, lineno, argv[0]);
+		break;
+	default:
+		tst_brk(TBROK, "%s:%d: %s failed (%d)", file, lineno, rc);
+	}
+}
+#define SAFE_RUNCMD(argv, stdout_path, stderr_path) \
+	safe_run_cmd(__FILE__, __LINE__, NULL, (argv), (stdout_path), (stderr_path))
+
 #endif /* SAFE_MACROS_H__ */
-- 
2.25.1

[LTP] [PATCH 2/2] Use SAFE_RUNCMD()

[Petr Vorel]

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
 testcases/kernel/syscalls/add_key/add_key05.c   | 15 ++-------------
 testcases/kernel/syscalls/quotactl/quotactl01.c | 14 ++------------
 testcases/kernel/syscalls/quotactl/quotactl06.c | 12 +-----------
 3 files changed, 5 insertions(+), 36 deletions(-)

diff --git a/testcases/kernel/syscalls/add_key/add_key05.c b/testcases/kernel/syscalls/add_key/add_key05.c
index a39bfa0b7..6a4e20564 100644
--- a/testcases/kernel/syscalls/add_key/add_key05.c
+++ b/testcases/kernel/syscalls/add_key/add_key05.c
@@ -36,19 +36,8 @@ 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, 1))) {
-	case 0:
-		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);
-	}
+
+	SAFE_RUNCMD(cmd_useradd, NULL, NULL);
 	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 ede61d7e4..30151814e 100644
--- a/testcases/kernel/syscalls/quotactl/quotactl01.c
+++ b/testcases/kernel/syscalls/quotactl/quotactl01.c
@@ -162,18 +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, 1);
-	switch (ret) {
-	case 0:
-		break;
-	case 255:
-		tst_brk(TCONF, "quotacheck binary not installed");
-		break;
-	default:
-		tst_brk(TBROK, "quotacheck exited with %i", ret);
-	}
+
+	SAFE_RUNCMD(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 a3b4517e0..0b62e6240 100644
--- a/testcases/kernel/syscalls/quotactl/quotactl06.c
+++ b/testcases/kernel/syscalls/quotactl/quotactl06.c
@@ -146,19 +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, 1);
-	switch (ret) {
-	case 0:
-		break;
-	case 255:
-		tst_brk(TCONF, "quotacheck binary not installed");
-		break;
-	default:
-		tst_brk(TBROK, "quotacheck exited with %i", ret);
-	}
+	SAFE_RUNCMD(cmd, NULL, NULL);
 
 	if (access(USRPATH, F_OK) == -1)
 		tst_brk(TFAIL | TERRNO, "user quotafile didn't exist");
-- 
2.25.1

[LTP] [PATCH 1/2] lib: Implement SAFE_RUN() macro (new API only)

[Yang Xu]

Hi Petr

There are some small nits
> Signed-off-by: Petr Vorel <pvorel@suse.cz>
> ---
>   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 b56f1a0f2..b8330801b 100644
> --- a/doc/test-writing-guidelines.txt
> +++ b/doc/test-writing-guidelines.txt
> @@ -1282,6 +1282,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_RUNCMD()' 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 80c4d9cb9..d701a003f 100644
> --- a/include/tst_safe_macros.h
> +++ b/include/tst_safe_macros.h
> @@ -547,4 +547,24 @@ 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, 1))) {
> +	case 0:
> +		break;
> +	case 255:
> +		tst_brk(TCONF, "%s:%d: %s not found in $PATH", file, lineno, argv[0]);
> +		break;
> +	default:
> +		tst_brk(TBROK, "%s:%d: %s failed (%d)", file, lineno, rc);
typo, rc -> rval and  miss  argv[0].
> +	}
> +}
> +#define SAFE_RUNCMD(argv, stdout_path, stderr_path) \
> +	safe_run_cmd(__FILE__, __LINE__, NULL, (argv), (stdout_path), (stderr_path))
I guess we don't need the third argument(NULL).

Best Regards
Yang Xu
> +
>   #endif /* SAFE_MACROS_H__ */
> 

[LTP] [PATCH 2/2] Use SAFE_RUNCMD()

[Yang Xu]

Hi Petr

This patch looks good to me, just a small nit
> Signed-off-by: Petr Vorel <pvorel@suse.cz>
> ---
>   testcases/kernel/syscalls/add_key/add_key05.c   | 15 ++-------------
>   testcases/kernel/syscalls/quotactl/quotactl01.c | 14 ++------------
>   testcases/kernel/syscalls/quotactl/quotactl06.c | 12 +-----------
>   3 files changed, 5 insertions(+), 36 deletions(-)
> 
> diff --git a/testcases/kernel/syscalls/add_key/add_key05.c b/testcases/kernel/syscalls/add_key/add_key05.c
> index a39bfa0b7..6a4e20564 100644
> --- a/testcases/kernel/syscalls/add_key/add_key05.c
> +++ b/testcases/kernel/syscalls/add_key/add_key05.c
> @@ -36,19 +36,8 @@ 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, 1))) {
> -	case 0:
> -		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);
> -	}
> +
> +	SAFE_RUNCMD(cmd_useradd, NULL, NULL);We should keep ltpuser and user_added assignment.

Best Regards
Yang Xu
>   	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 ede61d7e4..30151814e 100644
> --- a/testcases/kernel/syscalls/quotactl/quotactl01.c
> +++ b/testcases/kernel/syscalls/quotactl/quotactl01.c
> @@ -162,18 +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, 1);
> -	switch (ret) {
> -	case 0:
> -		break;
> -	case 255:
> -		tst_brk(TCONF, "quotacheck binary not installed");
> -		break;
> -	default:
> -		tst_brk(TBROK, "quotacheck exited with %i", ret);
> -	}
> +
> +	SAFE_RUNCMD(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 a3b4517e0..0b62e6240 100644
> --- a/testcases/kernel/syscalls/quotactl/quotactl06.c
> +++ b/testcases/kernel/syscalls/quotactl/quotactl06.c
> @@ -146,19 +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, 1);
> -	switch (ret) {
> -	case 0:
> -		break;
> -	case 255:
> -		tst_brk(TCONF, "quotacheck binary not installed");
> -		break;
> -	default:
> -		tst_brk(TBROK, "quotacheck exited with %i", ret);
> -	}
> +	SAFE_RUNCMD(cmd, NULL, NULL);
>   
>   	if (access(USRPATH, F_OK) == -1)
>   		tst_brk(TFAIL | TERRNO, "user quotafile didn't exist");
> 

[LTP] [PATCH 2/2] Use SAFE_RUNCMD()

[Petr Vorel]

Hi Xu,

> This patch looks good to me, just a small nit
> > Signed-off-by: Petr Vorel <pvorel@suse.cz>
> > ---
> >   testcases/kernel/syscalls/add_key/add_key05.c   | 15 ++-------------
> >   testcases/kernel/syscalls/quotactl/quotactl01.c | 14 ++------------
> >   testcases/kernel/syscalls/quotactl/quotactl06.c | 12 +-----------
> >   3 files changed, 5 insertions(+), 36 deletions(-)

> > diff --git a/testcases/kernel/syscalls/add_key/add_key05.c b/testcases/kernel/syscalls/add_key/add_key05.c
> > index a39bfa0b7..6a4e20564 100644
> > --- a/testcases/kernel/syscalls/add_key/add_key05.c
> > +++ b/testcases/kernel/syscalls/add_key/add_key05.c
> > @@ -36,19 +36,8 @@ 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, 1))) {
> > -	case 0:
> > -		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);
> > -	}
> > +
> > +	SAFE_RUNCMD(cmd_useradd, NULL, NULL);
> We should keep ltpuser and user_added assignment.

Oh, thanks for a reminder, sure.
+ there is an error in in safe_run_cmd() (used rc instead of rval).
I'll send v2.

Kind regards,
Petr

[LTP] [PATCH 2/2] Use SAFE_RUNCMD()

[Li Wang]

Hi Petr,

On Fri, Mar 20, 2020 at 9:50 PM Petr Vorel <pvorel@suse.cz> wrote:

> Signed-off-by: Petr Vorel <pvorel@suse.cz>
> ---
>  testcases/kernel/syscalls/add_key/add_key05.c   | 15 ++-------------
>  testcases/kernel/syscalls/quotactl/quotactl01.c | 14 ++------------
>  testcases/kernel/syscalls/quotactl/quotactl06.c | 12 +-----------


Apart from the three, do you consider converting to SAFE_RUNCMD for the
rest testcases?
(it seems not too much work remaining since only a few test case uses
tst_run_cmd)

kernel/syscalls/setpriority/setpriority01.c
kernel/syscalls/copy_file_range/copy_file_range02.c
...


-- 
Regards,
Li Wang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20200323/609ae5f1/attachment.htm>

[LTP] [PATCH 1/2] lib: Implement SAFE_RUN() macro (new API only)

[Petr Vorel]

Hi Xu,

> > +		tst_brk(TBROK, "%s:%d: %s failed (%d)", file, lineno, rc);
> typo, rc -> rval and  miss  argv[0].
> > +	}
> > +}
> > +#define SAFE_RUNCMD(argv, stdout_path, stderr_path) \
> > +	safe_run_cmd(__FILE__, __LINE__, NULL, (argv), (stdout_path), (stderr_path))
> I guess we don't need the third argument(NULL).

Thanks for catching these! I've sent v2 with your Reviewed-by: tag.

Kind regards,
Petr

[LTP] [PATCH 2/2] Use SAFE_RUNCMD()

[Yang Xu]

Hi Li


> Hi Petr,
> 
> On Fri, Mar 20, 2020 at 9:50 PM Petr Vorel <pvorel@suse.cz
> <mailto:pvorel@suse.cz>> wrote:
> 
>     Signed-off-by: Petr Vorel <pvorel@suse.cz <mailto:pvorel@suse.cz>>
>     ---
>      ?testcases/kernel/syscalls/add_key/add_key05.c? ?| 15 ++-------------
>      ?testcases/kernel/syscalls/quotactl/quotactl01.c | 14 ++------------
>      ?testcases/kernel/syscalls/quotactl/quotactl06.c | 12 +-----------
> 
> 
> Apart from the three, do you consider converting to SAFE_RUNCMD for the 
> rest testcases?
> (it seems not too much work remaining since only a few test case uses 
> tst_run_cmd)
At the beginning, I have the same idea. But after seeing code, I think 
we should not because these cases have many sub tests(only few test 
deponds on the result of the cmd execution.
> 
> kernel/syscalls/setpriority/setpriority01.c
One year ago has a commit db82b596(setpriority01: Skip only PRIO_USER 
when unable to add test user). It only affects PRIO_USER sub test.
> kernel/syscalls/copy_file_range/copy_file_range02.c
only affect test6 and test7
  6) Try to copy contents to a file chattred with +i
  *    flag -> EPERM
  * 7) Try to copy contents to a swapfile ->ETXTBSY


Best Regards
Yang Xu

> ...
> 
> 
> -- 
> Regards,
> Li Wang
> 
> 

[LTP] [PATCH 2/2] Use SAFE_RUNCMD()

[Petr Vorel]

Hi Li, Xu,

> >      ?testcases/kernel/syscalls/add_key/add_key05.c? ?| 15 ++-------------
> >      ?testcases/kernel/syscalls/quotactl/quotactl01.c | 14 ++------------
> >      ?testcases/kernel/syscalls/quotactl/quotactl06.c | 12 +-----------

> > Apart from the three, do you consider converting to SAFE_RUNCMD for the
> > rest testcases?
> > (it seems not too much work remaining since only a few test case uses
> > tst_run_cmd)
> At the beginning, I have the same idea. But after seeing code, I think we
> should not because these cases have many sub tests(only few test deponds on
> the result of the cmd execution.

> > kernel/syscalls/setpriority/setpriority01.c
> One year ago has a commit db82b596(setpriority01: Skip only PRIO_USER when
> unable to add test user). It only affects PRIO_USER sub test.
+ 1. I didn't want to break the case when useradd is not available (android or
some custom embedded linux) or there is no password file (root mounted as ro -
custom embedded linux).

BTW I also avoid handling adding user as I want to implement better handling
user and group in LTP (adding a flag), see:
https://github.com/linux-test-project/ltp/issues/468

Feel free to commend this plan.
This patchset is kind of preparation for it.

> > kernel/syscalls/copy_file_range/copy_file_range02.c
> only affect test6 and test7
>  6) Try to copy contents to a file chattred with +i
>  *    flag -> EPERM
>  * 7) Try to copy contents to a swapfile ->ETXTBSY
Yes, it'd be bad to break all tests due it.

Here is also problem with swapoff (or maybe chattr, mkswap, swapon; I don't
remember), which returns exit code 255 on error, so it's not possible to
distinguish this from the case whether command is not available (any idea, how
to fix it?).

Looking into old API uses, testcases/kernel/syscalls/swapon/libswapon.c could
use it (once it migrates to new API), although it uses tst_brkm(TFAIL).
testcases/kernel/input/input_helper.c probably not.

Kind regards,
Petr

[LTP] [PATCH 2/2] Use SAFE_RUNCMD()

[Li Wang]

Hi Petr, Xu,

On Mon, Mar 23, 2020 at 7:37 PM Petr Vorel <pvorel@suse.cz> wrote:

> Hi Li, Xu,
>
> > >       testcases/kernel/syscalls/add_key/add_key05.c   | 15
> ++-------------
> > >       testcases/kernel/syscalls/quotactl/quotactl01.c | 14
> ++------------
> > >       testcases/kernel/syscalls/quotactl/quotactl06.c | 12 +-----------
>
> > > Apart from the three, do you consider converting to SAFE_RUNCMD for the
> > > rest testcases?
> > > (it seems not too much work remaining since only a few test case uses
> > > tst_run_cmd)
> > At the beginning, I have the same idea. But after seeing code, I think we
> > should not because these cases have many sub tests(only few test deponds
> on
> > the result of the cmd execution.
>
> > > kernel/syscalls/setpriority/setpriority01.c
> > One year ago has a commit db82b596(setpriority01: Skip only PRIO_USER
> when
> > unable to add test user). It only affects PRIO_USER sub test.
> + 1. I didn't want to break the case when useradd is not available
> (android or
> some custom embedded linux) or there is no password file (root mounted as
> ro -
> custom embedded linux).
>

That's right. Thanks for the clarification.

>
> BTW I also avoid handling adding user as I want to implement better
> handling
> user and group in LTP (adding a flag), see:
> https://github.com/linux-test-project/ltp/issues/468


Good plan.


>
>
> Feel free to commend this plan.
> This patchset is kind of preparation for it.
>
> > > kernel/syscalls/copy_file_range/copy_file_range02.c
> > only affect test6 and test7
> >  6) Try to copy contents to a file chattred with +i
> >  *    flag -> EPERM
> >  * 7) Try to copy contents to a swapfile ->ETXTBSY
> Yes, it'd be bad to break all tests due it.
>
> Here is also problem with swapoff (or maybe chattr, mkswap, swapon; I don't
> remember), which returns exit code 255 on error, so it's not possible to
> distinguish this from the case whether command is not available (any idea,
> how
> to fix it?).
>

Maybe we could achieve a tst_cmd_available(char *cmd) in the C version?
which uses popen() to open a process like: "whereis/which command" and do
string parse in the result to see the path(/usr/bin/cmd, /usr/sbin/cmd) of
the bin if it has been found.

A draft version to show the idea:

int tst_cmd_available(char *cmd)
{
    int ret = 0;
    char path[PATH_MAX];
    char result[PATH_MAX];
    char command[PATH_MAX];

    snprintf(path, PATH_MAX, "/usr/bin/%s", cmd);
    snprintf(command, PATH_MAX, "whereis %s", cmd);
    FILE *fp = popen(command, "r");
    fgets(result, sizeof(result), fp);

    if (strstr(result, path) != NULL)
        ret = 1;
    pclose(fp);

    return ret;
}
-- 
Regards,
Li Wang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20200323/984d7b66/attachment.htm>

[LTP] [PATCH 2/2] Use SAFE_RUNCMD()

[Li Wang]

On Mon, Mar 23, 2020 at 9:42 PM Li Wang <liwang@redhat.com> wrote:

> Hi Petr, Xu,
>
> On Mon, Mar 23, 2020 at 7:37 PM Petr Vorel <pvorel@suse.cz> wrote:
>
>> Hi Li, Xu,
>>
>> > >       testcases/kernel/syscalls/add_key/add_key05.c   | 15
>> ++-------------
>> > >       testcases/kernel/syscalls/quotactl/quotactl01.c | 14
>> ++------------
>> > >       testcases/kernel/syscalls/quotactl/quotactl06.c | 12
>> +-----------
>>
>> > > Apart from the three, do you consider converting to SAFE_RUNCMD for
>> the
>> > > rest testcases?
>> > > (it seems not too much work remaining since only a few test case uses
>> > > tst_run_cmd)
>> > At the beginning, I have the same idea. But after seeing code, I think
>> we
>> > should not because these cases have many sub tests(only few test
>> deponds on
>> > the result of the cmd execution.
>>
>> > > kernel/syscalls/setpriority/setpriority01.c
>> > One year ago has a commit db82b596(setpriority01: Skip only PRIO_USER
>> when
>> > unable to add test user). It only affects PRIO_USER sub test.
>> + 1. I didn't want to break the case when useradd is not available
>> (android or
>> some custom embedded linux) or there is no password file (root mounted as
>> ro -
>> custom embedded linux).
>>
>
> That's right. Thanks for the clarification.
>
>>
>> BTW I also avoid handling adding user as I want to implement better
>> handling
>> user and group in LTP (adding a flag), see:
>> https://github.com/linux-test-project/ltp/issues/468
>
>
> Good plan.
>
>
>>
>>
>> Feel free to commend this plan.
>> This patchset is kind of preparation for it.
>>
>> > > kernel/syscalls/copy_file_range/copy_file_range02.c
>> > only affect test6 and test7
>> >  6) Try to copy contents to a file chattred with +i
>> >  *    flag -> EPERM
>> >  * 7) Try to copy contents to a swapfile ->ETXTBSY
>> Yes, it'd be bad to break all tests due it.
>>
>> Here is also problem with swapoff (or maybe chattr, mkswap, swapon; I
>> don't
>> remember), which returns exit code 255 on error, so it's not possible to
>> distinguish this from the case whether command is not available (any
>> idea, how
>> to fix it?).
>>
>
> Maybe we could achieve a tst_cmd_available(char *cmd) in the C version?
> which uses popen() to open a process like: "whereis/which command" and do
> string parse in the result to see the path(/usr/bin/cmd, /usr/sbin/cmd) of
> the bin if it has been found.
>

Or, simply to use access() if we gonna take care of embedded Linux, is this
reliable?

int tst_cmd_available(char *cmd)
{
    int ret = 0;
    char path[PATH_MAX];

    snprintf(path, PATH_MAX, "/usr/bin/%s", cmd);
    if (!access(path, X_OK)) {
        ret = 1;
        goto out;
    }

    snprintf(path, PATH_MAX, "/usr/sbin/%s", cmd);
    if (!access(path, X_OK)) {
        ret = 1;
        goto out;
    }

    snprintf(path, PATH_MAX, "/usr/local/bin/%s", cmd);
    if (!access(path, X_OK)) {
        ret = 1;
        goto out;
    }

    snprintf(path, PATH_MAX, "/usr/local/sbin/%s", cmd);
    if (!access(path, X_OK)) {
        ret = 1;
        goto out;
    }

out:
    return ret;
}
-- 
Regards,
Li Wang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20200323/6d7d726d/attachment-0001.htm>

[LTP] [PATCH 2/2] Use SAFE_RUNCMD()

[Petr Vorel]

Hi Li,

...
> > > > kernel/syscalls/copy_file_range/copy_file_range02.c
> > > only affect test6 and test7
> > >  6) Try to copy contents to a file chattred with +i
> > >  *    flag -> EPERM
> > >  * 7) Try to copy contents to a swapfile ->ETXTBSY
> > Yes, it'd be bad to break all tests due it.

> > Here is also problem with swapoff (or maybe chattr, mkswap, swapon; I don't
> > remember), which returns exit code 255 on error, so it's not possible to
> > distinguish this from the case whether command is not available (any idea,
> > how
> > to fix it?).

> Maybe we could achieve a tst_cmd_available(char *cmd) in the C version?
> which uses popen() to open a process like: "whereis/which command" and do
> string parse in the result to see the path(/usr/bin/cmd, /usr/sbin/cmd) of
> the bin if it has been found.
Or how about loop whole path like whereis/which command? I want to cover also
these "strange systems" (Android and embedded).

I wonder if to use this all the time (e.g. in safe_run_cmd(), because solution
in tst_run_cmd_fds_() (errno == ENOENT) works most of the time. Maybe changing
exit code 255 to something less common (e.g. INT_MAX - 5).
Do you want to use tst_cmd_available() also  not only as API

Kind regards,
Petr

> A draft version to show the idea:

> int tst_cmd_available(char *cmd)
> {
>     int ret = 0;
>     char path[PATH_MAX];
>     char result[PATH_MAX];
>     char command[PATH_MAX];

>     snprintf(path, PATH_MAX, "/usr/bin/%s", cmd);
>     snprintf(command, PATH_MAX, "whereis %s", cmd);
>     FILE *fp = popen(command, "r");
>     fgets(result, sizeof(result), fp);

>     if (strstr(result, path) != NULL)
>         ret = 1;
>     pclose(fp);

>     return ret;
> }

[LTP] [PATCH 2/2] Use SAFE_RUNCMD()

[Petr Vorel]

Hi Li,

> Or, simply to use access() if we gonna take care of embedded Linux, is this
> reliable?

> int tst_cmd_available(char *cmd)
> {
>     int ret = 0;
>     char path[PATH_MAX];

>     snprintf(path, PATH_MAX, "/usr/bin/%s", cmd);
>     if (!access(path, X_OK)) {
>         ret = 1;
>         goto out;
>     }

>     snprintf(path, PATH_MAX, "/usr/sbin/%s", cmd);
>     if (!access(path, X_OK)) {
>         ret = 1;
>         goto out;
>     }

>     snprintf(path, PATH_MAX, "/usr/local/bin/%s", cmd);
>     if (!access(path, X_OK)) {
>         ret = 1;
>         goto out;
>     }

>     snprintf(path, PATH_MAX, "/usr/local/sbin/%s", cmd);
>     if (!access(path, X_OK)) {
>         ret = 1;
>         goto out;
>     }

> out:
>     return ret;
> }

Something like this would work on whole PATH.
It's just a question if we want to use it.

int tst_cmd_available(char *cmd)
{
	char *dup = strdup(getenv("PATH"));
	char *s = dup;
	char *p = NULL;
	int ret = 0;
    char path[PATH_MAX];

	do {
		p = strchr(s, ':');
		if (p != NULL) {
			p[0] = 0;
		}
		snprintf(path, PATH_MAX, "%s/%s", s, cmd);

		if (!access(path, X_OK)) {
			ret = 1;
			break;
		}
		s = p + 1;
	} while (p != NULL);

	free(dup);
	return ret;
}

Kind regards,
Petr

[LTP] [PATCH 2/2] Use SAFE_RUNCMD()

[Li Wang]

Hi Petr,

On Mon, Mar 23, 2020 at 11:49 PM Petr Vorel <pvorel@suse.cz> wrote:

> Hi Li,
>
> ...
> > > > > kernel/syscalls/copy_file_range/copy_file_range02.c
> > > > only affect test6 and test7
> > > >  6) Try to copy contents to a file chattred with +i
> > > >  *    flag -> EPERM
> > > >  * 7) Try to copy contents to a swapfile ->ETXTBSY
> > > Yes, it'd be bad to break all tests due it.
>
> > > Here is also problem with swapoff (or maybe chattr, mkswap, swapon; I
> don't
> > > remember), which returns exit code 255 on error, so it's not possible
> to
> > > distinguish this from the case whether command is not available (any
> idea,
> > > how
> > > to fix it?).
>
> > Maybe we could achieve a tst_cmd_available(char *cmd) in the C version?
> > which uses popen() to open a process like: "whereis/which command" and do
> > string parse in the result to see the path(/usr/bin/cmd, /usr/sbin/cmd)
> of
> > the bin if it has been found.
> Or how about loop whole path like whereis/which command? I want to cover
> also
> these "strange systems" (Android and embedded).
>

Yes, looping the whole PATH is necessary. I'd like to go the access() way
which you improved in the last email.


>
> I wonder if to use this all the time (e.g. in safe_run_cmd(), because
> solution
> in tst_run_cmd_fds_() (errno == ENOENT) works most of the time. Maybe
> changing
> exit code 255 to something less common (e.g. INT_MAX - 5).
>

That might be feasible. I hope to hear more voices from others here.
@Cyril @Xu what do you think?


> Do you want to use tst_cmd_available() also  not only as API
>

Hmm. I guess we can add a new filed in tst_test struct as '.cmd_check' to
make the command support checking automatically in the setup() phase.
That's not very needed but better to testcase manage if we do.

-- 
Regards,
Li Wang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20200324/41499d34/attachment.htm>

[LTP] [PATCH 2/2] Use SAFE_RUNCMD()

[Petr Vorel]

Hi Cyril,

...
> We already do have tst_get_path() that does more or less the same.
Thanks for info!

> Also if we are going to add this functionality it should be added as an
> .needs_cmds array in the tst_test structure.
.needs_cmds sounds as a good idea. But let's do it as a separate effort.
I'll leave already sent v2 for review. Once .needs_cmds is implemented, we can
use it as well for copy_file_range02.c.

BTW what do you think on changing 255 (and 254) for something less common?
It's just a corner case swapon on certain setup in copy_file_range02.c returns
255 on error:

Setting up swapspace version 1, size = 36 KiB (36864 bytes)
no label, UUID=bae78639-be0b-42b2-9e91-815b05f5751b
swapon: /tmp/msT4Ch/file_swap: swapon failed: Invalid argument
copy_file_range02.c:95: CONF: swapon binary not installed or failed

Kind regards,
Petr

[LTP] [PATCH 2/2] Use SAFE_RUNCMD()

[Petr Vorel]

Hi Cyril,

> > > Also if we are going to add this functionality it should be added as an
> > > .needs_cmds array in the tst_test structure.
> > .needs_cmds sounds as a good idea. But let's do it as a separate effort.
> > I'll leave already sent v2 for review. Once .needs_cmds is implemented, we can
> > use it as well for copy_file_range02.c.

> 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.
Oh yes, metadata effort, that's important, I'll implement it. But I still think
it's useful to have SAFE_RUNCMD(), although I can remove TCONF (use flag
TST_RUN_CMD_CHECK_CMD, see below).

> > BTW what do you think on changing 255 (and 254) for something less common?
> > It's just a corner case swapon on certain setup in copy_file_range02.c returns
> > 255 on error:

> I do not think that this will solve the problem. We may hit the same
> problem with any random number we will choose.

> I guess checking for the command existence before we vfork() would be
> safer bet here.
+1. But this IMHO requires to add another parameter to tst_run_cmd_fds_(),
because we don't want always TCONF. Best will be to turn int pass_exit_val into
int flags with 2 possible values (e.g. TST_RUN_CMD_PASS_EXIT_VAL and
TST_RUN_CMD_CHECK_CMD).

> > Setting up swapspace version 1, size = 36 KiB (36864 bytes)
> > no label, UUID=bae78639-be0b-42b2-9e91-815b05f5751b
> > swapon: /tmp/msT4Ch/file_swap: swapon failed: Invalid argument
> > copy_file_range02.c:95: CONF: swapon binary not installed or failed

Kind regards,
Petr

[LTP] [PATCH 2/2] Use SAFE_RUNCMD()

[Cyril Hrubis]

Hi!
> Something like this would work on whole PATH.
> It's just a question if we want to use it.
> 
> int tst_cmd_available(char *cmd)
> {
> 	char *dup = strdup(getenv("PATH"));
> 	char *s = dup;
> 	char *p = NULL;
> 	int ret = 0;
>     char path[PATH_MAX];
> 
> 	do {
> 		p = strchr(s, ':');
> 		if (p != NULL) {
> 			p[0] = 0;
> 		}
> 		snprintf(path, PATH_MAX, "%s/%s", s, cmd);
> 
> 		if (!access(path, X_OK)) {
> 			ret = 1;
> 			break;
> 		}
> 		s = p + 1;
> 	} while (p != NULL);
> 
> 	free(dup);
> 	return ret;
> }

We already do have tst_get_path() that does more or less the same.

Also if we are going to add this functionality it should be added as an
.needs_cmds array in the tst_test structure.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH 2/2] Use SAFE_RUNCMD()

[Cyril Hrubis]

Hi!
> > Also if we are going to add this functionality it should be added as an
> > .needs_cmds array in the tst_test structure.
> .needs_cmds sounds as a good idea. But let's do it as a separate effort.
> I'll leave already sent v2 for review. Once .needs_cmds is implemented, we can
> use it as well for copy_file_range02.c.

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.

> BTW what do you think on changing 255 (and 254) for something less common?
> It's just a corner case swapon on certain setup in copy_file_range02.c returns
> 255 on error:

I do not think that this will solve the problem. We may hit the same
problem with any random number we will choose.

I guess checking for the command existence before we vfork() would be
safer bet here.

> Setting up swapspace version 1, size = 36 KiB (36864 bytes)
> no label, UUID=bae78639-be0b-42b2-9e91-815b05f5751b
> swapon: /tmp/msT4Ch/file_swap: swapon failed: Invalid argument
> copy_file_range02.c:95: CONF: swapon binary not installed or failed
> 
> Kind regards,
> Petr

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH 2/2] Use SAFE_RUNCMD()

[Li Wang]

On Wed, Mar 25, 2020 at 1:57 AM Cyril Hrubis <chrubis@suse.cz> wrote:

> Hi!
> > > Also if we are going to add this functionality it should be added as an
> > > .needs_cmds array in the tst_test structure.
> > .needs_cmds sounds as a good idea. But let's do it as a separate effort.
>
+1, thanks Petr!


> > I'll leave already sent v2 for review. Once .needs_cmds is implemented,
> we can
> > use it as well for copy_file_range02.c.
>
> 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.
>

Sounds good.

And this makes me think more of the '.request_hugepages' story. The
needs_foo flags require the foo to be present on the system as hard
requirements. In some situations(i.e copy_file_range02.c), we probably need
to handle the soft situation, which means, the commands are only part of
the test requirement. So if it writing with .needs_cmds="xxx", it might
skip the whole test in setup() phase.

So things are clear now, checking for the command existence in
tst_run_cmd_fds_() is really necessary. For the test with "cmds" is needed
just adding the .needs_cmds="xxx", for the test with "cmds" is only part of
it, we can avoid writing '.needs_cmds' but calling tst_run_cmd() function
directly.

-- 
Regards,
Li Wang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20200325/6968ec47/attachment-0001.htm>

[LTP] [PATCH 2/2] Use SAFE_RUNCMD()

[Li Wang]

Hi Petr,

On Wed, Mar 25, 2020 at 2:55 AM Petr Vorel <pvorel@suse.cz> wrote:

> Hi Cyril,
>
> > > > Also if we are going to add this functionality it should be added as
> an
> > > > .needs_cmds array in the tst_test structure.
> > > .needs_cmds sounds as a good idea. But let's do it as a separate
> effort.
> > > I'll leave already sent v2 for review. Once .needs_cmds is
> implemented, we can
> > > use it as well for copy_file_range02.c.
>
> > 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.
> Oh yes, metadata effort, that's important, I'll implement it. But I still
> think
> it's useful to have SAFE_RUNCMD(), although I can remove TCONF (use flag
> TST_RUN_CMD_CHECK_CMD, see below).
>

I'm OK with SAFE_RUNCMD.


> > > BTW what do you think on changing 255 (and 254) for something less
> common?
> > > It's just a corner case swapon on certain setup in copy_file_range02.c
> returns
> > > 255 on error:
>
> > I do not think that this will solve the problem. We may hit the same
> > problem with any random number we will choose.
>
> > I guess checking for the command existence before we vfork() would be
> > safer bet here.
> +1. But this IMHO requires to add another parameter to tst_run_cmd_fds_(),
> because we don't want always TCONF. Best will be to turn int pass_exit_val
> into
> int flags with 2 possible values (e.g. TST_RUN_CMD_PASS_EXIT_VAL and
> TST_RUN_CMD_CHECK_CMD).
>

Agree, if we set '.needs_cmds' already we don't need to double-check the
command existence in tst_run_cmd() again.

-- 
Regards,
Li Wang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20200325/f87216f7/attachment.htm>

[LTP] [PATCH 2/2] Use SAFE_RUNCMD()

[Petr Vorel]

Hi Li, Metan,

> > And this makes me think more of the '.request_hugepages' story. The
> > needs_foo flags require the foo to be present on the system as hard
> > requirements. In some situations(i.e copy_file_range02.c), we probably need
> > to handle the soft situation, which means, the commands are only part of
> > the test requirement. So if it writing with .needs_cmds="xxx", it might
> > skip the whole test in setup() phase.
+1. This is similar to a general problem how to structure tests when you want to
use tst_brk() and cleanup function (having more unrelated tests in single C file
means one should try to avoid using tst_brk() when not needed).

> Indeed, there are couple of solutions for that, one of them would have
> all the arrays doubled and one of them would list hard requirement while
> the other soft requirements. Then we will end up with something as
> "need_cmds" and "wants_cmds". The second one would be more or less
> informative, the test may print a message "Missing foo command test
> coverage will be limited".
I was thinking about it and thought that would be too rich API (given there is
not that much external dependencies for C tests). But ok, sounds reasonable.

Also similar use case from shell tests: mostly $TST_NEEDS_CMDS is used,
which stop whole testing. But rarely (only in 3 tests and tst_net.sh) is used
tst_require_cmds() directly - it's a hard requirement, but it tries to run some
test before (or require it only when it's needed - tst_net.sh).
But that's bad from metadata point of view (you concentrate on metadata in C,
but sooner or later we'll need to handle shell as well).

Kind regards,
Petr

[LTP] [PATCH 2/2] Use SAFE_RUNCMD()

[Cyril Hrubis]

Hi!
> > > And this makes me think more of the '.request_hugepages' story. The
> > > needs_foo flags require the foo to be present on the system as hard
> > > requirements. In some situations(i.e copy_file_range02.c), we probably need
> > > to handle the soft situation, which means, the commands are only part of
> > > the test requirement. So if it writing with .needs_cmds="xxx", it might
> > > skip the whole test in setup() phase.
> +1. This is similar to a general problem how to structure tests when you want to
> use tst_brk() and cleanup function (having more unrelated tests in single C file
> means one should try to avoid using tst_brk() when not needed).
> 
> > Indeed, there are couple of solutions for that, one of them would have
> > all the arrays doubled and one of them would list hard requirement while
> > the other soft requirements. Then we will end up with something as
> > "need_cmds" and "wants_cmds". The second one would be more or less
> > informative, the test may print a message "Missing foo command test
> > coverage will be limited".
> I was thinking about it and thought that would be too rich API (given there is
> not that much external dependencies for C tests). But ok, sounds reasonable.

Well yes, the matrix of options would explode exponentially if we do not
keep it reasonable, hence we should keep it as simple as possible. So
unless there is a real need for the wants_cmds I wouldn't add it now.

> Also similar use case from shell tests: mostly $TST_NEEDS_CMDS is used,
> which stop whole testing. But rarely (only in 3 tests and tst_net.sh) is used
> tst_require_cmds() directly - it's a hard requirement, but it tries to run some
> test before (or require it only when it's needed - tst_net.sh).
> But that's bad from metadata point of view (you concentrate on metadata in C,
> but sooner or later we'll need to handle shell as well).

Unfortunatelly the whole problem is more complex than set of flags. The
dependencies are often modified by the system properties, test command
lline options, etc. Complete solution would need to be a set of
conditionals or a domain language that would be able to express all
dependencies. The main problem is that this would soon get too complex
to a point where people would struggle to write the dependency rules.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH 2/2] Use SAFE_RUNCMD()

[Yang Xu]

Hi Petr,Li, Cyril

> Hi Li, Metan,
> 
>>> And this makes me think more of the '.request_hugepages' story. The
>>> needs_foo flags require the foo to be present on the system as hard
>>> requirements. In some situations(i.e copy_file_range02.c), we probably need
>>> to handle the soft situation, which means, the commands are only part of
>>> the test requirement. So if it writing with .needs_cmds="xxx", it might
>>> skip the whole test in setup() phase.
> +1. This is similar to a general problem how to structure tests when you want to
> use tst_brk() and cleanup function (having more unrelated tests in single C file
> means one should try to avoid using tst_brk() when not needed).
> 
>> Indeed, there are couple of solutions for that, one of them would have
>> all the arrays doubled and one of them would list hard requirement while
>> the other soft requirements. Then we will end up with something as
>> "need_cmds" and "wants_cmds". The second one would be more or less
>> informative, the test may print a message "Missing foo command test
>> coverage will be limited".
> I was thinking about it and thought that would be too rich API (given there is
> not that much external dependencies for C tests). But ok, sounds reasonable.
> 
> Also similar use case from shell tests: mostly $TST_NEEDS_CMDS is used,
> which stop whole testing. But rarely (only in 3 tests and tst_net.sh) is used
> tst_require_cmds() directly - it's a hard requirement, but it tries to run some
> test before (or require it only when it's needed - tst_net.sh).
> But that's bad from metadata point of view (you concentrate on metadata in C,
> but sooner or later we'll need to handle shell as well).
I have seen the history about this problem. We have few C cases to use 
many commands(copy_file_range02.c is a specify case, I ported it from 
xfstest to increase coverage), do we really want to implement need_cmd 
or want_cmds(Usually, we seldom use command in c case and  we should 
avoid this for reduce unnecessary dependencies, except user level 
command such as mkfs or makeswap or useradd)? It will give user a mislead.

ps:copy_file_range02.c should use swapon and swapoff syscall instead of 
command.

Best Regards
Yang Xu
> 
> Kind regards,
> Petr
> 
> 

[LTP] [PATCH 2/2] Use SAFE_RUNCMD()

[Petr Vorel]

Hi,

...
> > > Indeed, there are couple of solutions for that, one of them would have
> > > all the arrays doubled and one of them would list hard requirement while
> > > the other soft requirements. Then we will end up with something as
> > > "need_cmds" and "wants_cmds". The second one would be more or less
> > > informative, the test may print a message "Missing foo command test
> > > coverage will be limited".
> > I was thinking about it and thought that would be too rich API (given there is
> > not that much external dependencies for C tests). But ok, sounds reasonable.

> Well yes, the matrix of options would explode exponentially if we do not
> keep it reasonable, hence we should keep it as simple as possible. So
> unless there is a real need for the wants_cmds I wouldn't add it now.
+1.

> > Also similar use case from shell tests: mostly $TST_NEEDS_CMDS is used,
> > which stop whole testing. But rarely (only in 3 tests and tst_net.sh) is used
> > tst_require_cmds() directly - it's a hard requirement, but it tries to run some
> > test before (or require it only when it's needed - tst_net.sh).
> > But that's bad from metadata point of view (you concentrate on metadata in C,
> > but sooner or later we'll need to handle shell as well).

> Unfortunatelly the whole problem is more complex than set of flags. The
> dependencies are often modified by the system properties, test command
> lline options, etc. Complete solution would need to be a set of
> conditionals or a domain language that would be able to express all
> dependencies. The main problem is that this would soon get too complex
> to a point where people would struggle to write the dependency rules.
OK, I don't know the details for metadata project. I thought that you're
planning to parse struct tst_test to get metadata for C. So my assumption was
that parsing TST_* would be equivalent for shell.

Kind regards,
Petr

[LTP] [PATCH 2/2] Use SAFE_RUNCMD()

[Petr Vorel]

Hi Xu,

> I have seen the history about this problem. We have few C cases to use many
> commands(copy_file_range02.c is a specify case, I ported it from xfstest to
> increase coverage), do we really want to implement need_cmd or
> want_cmds(Usually, we seldom use command in c case and  we should avoid this
> for reduce unnecessary dependencies, except user level command such as mkfs
> or makeswap or useradd)? It will give user a mislead.

> ps:copy_file_range02.c should use swapon and swapoff syscall instead of
> command.
Yes, rewriting to C would be an improvement for non-standard linux platforms
(but then you need to deal with other exceptions: e.g. whether
swapon()/swapoff() is even supported on platforms like Android and you might
endup with 1) much more code 2) TCONF anyway for these platforms.
And there is also chattr and mkswap.

Besides this IMHO there will always be a need for running some command via
tst_run_cmd() in the test instead of reimplementing a wheel. Some of other
dependencies:

cat (testcases/cve/stack_clash.c this one could be using C code),
mpdprobe, make, mkswap, quotacheck,
useradd/userdel (I plan to put these into the library, but still it's much
easier to use them than reimplement code in C).

Also library itself (these will not use the flag): insmod, modprobe, rmmod,  mkfs.*,
systemd-detect-virt (this one is not a hard dependency).

Kind regards,
Petr

[LTP] [PATCH 2/2] Use SAFE_RUNCMD()

[Cyril Hrubis]

Hi!
> > > > Also if we are going to add this functionality it should be added as an
> > > > .needs_cmds array in the tst_test structure.
> > > .needs_cmds sounds as a good idea. But let's do it as a separate effort.
> >
> +1, thanks Petr!
> 
> 
> > > I'll leave already sent v2 for review. Once .needs_cmds is implemented,
> > we can
> > > use it as well for copy_file_range02.c.
> >
> > 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.
> >
> 
> Sounds good.
> 
> And this makes me think more of the '.request_hugepages' story. The
> needs_foo flags require the foo to be present on the system as hard
> requirements. In some situations(i.e copy_file_range02.c), we probably need
> to handle the soft situation, which means, the commands are only part of
> the test requirement. So if it writing with .needs_cmds="xxx", it might
> skip the whole test in setup() phase.

Indeed, there are couple of solutions for that, one of them would have
all the arrays doubled and one of them would list hard requirement while
the other soft requirements. Then we will end up with something as
"need_cmds" and "wants_cmds". The second one would be more or less
informative, the test may print a message "Missing foo command test
coverage will be limited".

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH 2/2] Use SAFE_RUNCMD()

[Li Wang]

On Wed, Mar 25, 2020 at 11:57 PM Petr Vorel <pvorel@suse.cz> wrote:

> Hi Xu,
>
> > I have seen the history about this problem. We have few C cases to use
> many
> > commands(copy_file_range02.c is a specify case, I ported it from xfstest
> to
> > increase coverage), do we really want to implement need_cmd or
> > want_cmds(Usually, we seldom use command in c case and  we should avoid
> this
> > for reduce unnecessary dependencies, except user level command such as
> mkfs
> > or makeswap or useradd)? It will give user a mislead.
>
> > ps:copy_file_range02.c should use swapon and swapoff syscall instead of
> > command.
> Yes, rewriting to C would be an improvement for non-standard linux
> platforms
> (but then you need to deal with other exceptions: e.g. whether
> swapon()/swapoff() is even supported on platforms like Android and you
> might
> endup with 1) much more code 2) TCONF anyway for these platforms.
> And there is also chattr and mkswap.
>

That's right. Rewrite into C can't solve the problem by the roots.


>
> Besides this IMHO there will always be a need for running some command via
> tst_run_cmd() in the test instead of reimplementing a wheel. Some of other
> dependencies:
>

Agree. We at least need achieve the tst_cmd_available() for tst_run_cmd()
or .needs_cmds, let's try it together in code first then help to find best
solution in patch polishing.


>
> cat (testcases/cve/stack_clash.c this one could be using C code),
> mpdprobe, make, mkswap, quotacheck,
> useradd/userdel (I plan to put these into the library, but still it's much
> easier to use them than reimplement code in C).
>
> Also library itself (these will not use the flag): insmod, modprobe,
> rmmod,  mkfs.*,
> systemd-detect-virt (this one is not a hard dependency).
>
> Kind regards,
> Petr
>
>

-- 
Regards,
Li Wang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20200326/be4ce710/attachment.htm>

[LTP] [PATCH 2/2] Use SAFE_RUNCMD()

[Yang Xu]

Hi Petr

> Hi Xu,
> 
>> I have seen the history about this problem. We have few C cases to use many
>> commands(copy_file_range02.c is a specify case, I ported it from xfstest to
>> increase coverage), do we really want to implement need_cmd or
>> want_cmds(Usually, we seldom use command in c case and  we should avoid this
>> for reduce unnecessary dependencies, except user level command such as mkfs
>> or makeswap or useradd)? It will give user a mislead.
> 
>> ps:copy_file_range02.c should use swapon and swapoff syscall instead of
>> command.
> Yes, rewriting to C would be an improvement for non-standard linux platforms
> (but then you need to deal with other exceptions: e.g. whether
> swapon()/swapoff() is even supported on platforms like Android and you might
> endup with 1) much more code 2) TCONF anyway for these platforms.
> And there is also chattr and mkswap.
> 
I missed non-standard linux platforms.
> Besides this IMHO there will always be a need for running some command via
> tst_run_cmd() in the test instead of reimplementing a wheel.
Yes, we can not avoid using command.
> Some of other
> dependencies:
> 
> cat (testcases/cve/stack_clash.c this one could be using C code),
> mpdprobe, make, mkswap, quotacheck,
> useradd/userdel (I plan to put these into the library, but still it's much
> easier to use them than reimplement code in C).
Yes, it is fast and efficient.

Also as Li said, we can write code firstly(add_key05 can use library a 
api int the future as your issue#468 mentioned).

Best Regards
Yang Xu
> 
> Also library itself (these will not use the flag): insmod, modprobe, rmmod,  mkfs.*,
> systemd-detect-virt (this one is not a hard dependency).
> 
> Kind regards,
> Petr
> 
>