[LTP] [RFC PATCH 0/3] C, shell API: Add $LTPROOT/testcases/bin into PATH

[LTP] [RFC PATCH 0/3] C, shell API: Add $LTPROOT/testcases/bin into PATH

[Petr Vorel]

Hi,

although we need to set $LTPROOT/testcases/bin in LTP runner
(i.e. keep it in runltp and runltp-ng), we can add this path when
running from installed LTP (to simplify normal setup).

We could rethink running from git tree (setting TST_DATAROOT),
or adding variable for remote LTP installation location for SSH based
network tests. But that's another step.

Kind regards,
Petr

Petr Vorel (3):
  tst_test.sh: Add $LTPROOT/testcases/bin into PATH
  lib: Add $LTPROOT/testcases/bin into PATH
  doc: Update LTPROOT and PATH environment variables

 README.md                 |  2 +-
 doc/user-guide.txt        |  5 +++--
 lib/tst_test.c            | 22 +++++++++++++++++++---
 testcases/lib/tst_test.sh |  4 ++++
 4 files changed, 27 insertions(+), 6 deletions(-)

-- 
2.32.0

[LTP] [RFC PATCH 1/3] tst_test.sh: Add $LTPROOT/testcases/bin into PATH

[Petr Vorel]

when LTPROOT set. Also TWARN when directory does not exist.

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
 testcases/lib/tst_test.sh | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/testcases/lib/tst_test.sh b/testcases/lib/tst_test.sh
index 8b4e9cb60..970eca7d1 100644
--- a/testcases/lib/tst_test.sh
+++ b/testcases/lib/tst_test.sh
@@ -697,6 +697,10 @@ if [ -z "$LTPROOT" ]; then
 	export LTPROOT="$PWD"
 	export TST_DATAROOT="$LTPROOT/datafiles"
 else
+	if [ ! -d "$LTPROOT/testcases/bin" ]; then
+		tst_res TWARN "$LTPROOT/testcases/bin does not exist, is \$LTPROOT properly set?"
+	fi
+	export PATH="$LTPROOT/testcases/bin:$PATH"
 	export TST_DATAROOT="$LTPROOT/testcases/data/$TST_ID"
 fi
 
-- 
2.32.0

[LTP] [RFC PATCH 2/3] lib: Add $LTPROOT/testcases/bin into PATH

[Petr Vorel]

when LTPROOT set. Put it as the last.
TWARN when directory does not exist.

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
 lib/tst_test.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/lib/tst_test.c b/lib/tst_test.c
index 36a4809c7..14a739eb5 100644
--- a/lib/tst_test.c
+++ b/lib/tst_test.c
@@ -1149,15 +1149,31 @@ static unsigned long long get_time_ms(void)
 static void add_paths(void)
 {
 	char *old_path = getenv("PATH");
+	const char *ltproot = getenv("LTPROOT");
 	const char *start_dir;
-	char *new_path;
+	char *bin_dir, *new_path = NULL;
 
 	start_dir = tst_get_startwd();
 
+	/* : - current directory */
 	if (old_path)
-		SAFE_ASPRINTF(&new_path, "%s::%s", old_path, start_dir);
+		SAFE_ASPRINTF(&new_path, "%s:", old_path);
 	else
-		SAFE_ASPRINTF(&new_path, "::%s", start_dir);
+		strcat(new_path, ":");
+
+	/* empty for library C API tests */
+	if (strlen(start_dir) > 0)
+		SAFE_ASPRINTF(&new_path, "%s:%s", new_path, start_dir);
+
+	if (ltproot) {
+		SAFE_ASPRINTF(&bin_dir, "%s/testcases/bin", ltproot);
+
+		if (access(bin_dir, F_OK) == -1)
+			tst_res(TWARN, "'%s' does not exist, is $LTPROOT set correctly?",
+				bin_dir);
+		else
+			SAFE_ASPRINTF(&new_path, "%s:%s", new_path, bin_dir);
+	}
 
 	SAFE_SETENV("PATH", new_path, 1);
 	free(new_path);
-- 
2.32.0

[LTP] [RFC PATCH 2/3] lib: Add $LTPROOT/testcases/bin into PATH

[Cyril Hrubis]

Hi!
> diff --git a/lib/tst_test.c b/lib/tst_test.c
> index 36a4809c7..14a739eb5 100644
> --- a/lib/tst_test.c
> +++ b/lib/tst_test.c
> @@ -1149,15 +1149,31 @@ static unsigned long long get_time_ms(void)
>  static void add_paths(void)
>  {
>  	char *old_path = getenv("PATH");
> +	const char *ltproot = getenv("LTPROOT");
>  	const char *start_dir;
> -	char *new_path;
> +	char *bin_dir, *new_path = NULL;
>  
>  	start_dir = tst_get_startwd();
>  
> +	/* : - current directory */
>  	if (old_path)
> -		SAFE_ASPRINTF(&new_path, "%s::%s", old_path, start_dir);
> +		SAFE_ASPRINTF(&new_path, "%s:", old_path);
>  	else
> -		SAFE_ASPRINTF(&new_path, "::%s", start_dir);
> +		strcat(new_path, ":");

I personally think that strcat() function is broken be desing and that
we should avoid using it.

Also in this place is guaranteed SEGFAULT since you strcat() to NULL.

> +	/* empty for library C API tests */
> +	if (strlen(start_dir) > 0)
> +		SAFE_ASPRINTF(&new_path, "%s:%s", new_path, start_dir);
                                ^
				This is a memory leak.

As far as I can tell the asprintf() does not free th strp if non-NULL.

> +	if (ltproot) {
> +		SAFE_ASPRINTF(&bin_dir, "%s/testcases/bin", ltproot);
> +
> +		if (access(bin_dir, F_OK) == -1)
> +			tst_res(TWARN, "'%s' does not exist, is $LTPROOT set correctly?",
> +				bin_dir);
> +		else
> +			SAFE_ASPRINTF(&new_path, "%s:%s", new_path, bin_dir);
                                          ^
					  And this as well.
> +	}

I guess that we can also fairly simplify the code by expecting that PATH
is never unset from the start, maybe we should just check it and WARN if
it's not. Also we can assume that if LTPROOT is set we do not have to
add the start_dir since the start_dir is only useful when tests are
executed from the git checkout.

Something as:

	const char *old_path = getenv("PATH");
	const char *ltproot = getenv("LTPROOT");
	const char *start_dir = tst_get_startwd();
	char *new_path;

	if (!old_path) {
		tst_res(TWARN, "\$PATH not set!");
		return;
	}

	if (ltproot)
		SAFE_ASPRINTF(&new_path, "%s::%s/testcases/bin/", old_path, ltproot);
	else
		SAFE_ASPRINTF(&new_path, "%s::%s", old_path, start_dir);


>  	SAFE_SETENV("PATH", new_path, 1);
>  	free(new_path);
> -- 
> 2.32.0
> 

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [RFC PATCH 2/3] lib: Add $LTPROOT/testcases/bin into PATH

[Petr Vorel]

Hi Cyril,

> I guess that we can also fairly simplify the code by expecting that PATH
> is never unset from the start, maybe we should just check it and WARN if
> it's not. Also we can assume that if LTPROOT is set we do not have to
> add the start_dir since the start_dir is only useful when tests are
> executed from the git checkout.

I'm sorry to sent patchset with full of bugs, thanks for all your explanation.
While your suggestion could work, it's a question if my effort help to anything
(as Li noted). My intend was to require only LTPROOT, but even we expect
script/binary is called by full path (otherwise adjusting PATH would be required
anyway) fix work only for C API. tst_test.sh PATH is missing for shell API :(.
(discussed with Li under 3rd patch)

Kind regards,
Petr

[LTP] [RFC PATCH 3/3] doc: Update LTPROOT and PATH environment variables

[Petr Vorel]

to address changes in two previous commits.

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
 README.md          | 2 +-
 doc/user-guide.txt | 7 +++++--
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/README.md b/README.md
index 703395c6b..b0ad47dfa 100644
--- a/README.md
+++ b/README.md
@@ -162,7 +162,7 @@ $ testcases/bin/ioctl01 -h
 Many require certain environment variables to be set
 
 ```
-$ LTPROOT=/opt/ltp PATH="$PATH:$LTPROOT/testcases/bin" testcases/bin/wc01.sh
+$ LTPROOT=/opt/ltp testcases/bin/wc01.sh
 ```
 
 Most commonly, the path variable needs to be set and also `LTPROOT`, but there
diff --git a/doc/user-guide.txt b/doc/user-guide.txt
index 8df29e688..ab2d773a7 100644
--- a/doc/user-guide.txt
+++ b/doc/user-guide.txt
@@ -11,6 +11,7 @@ For running LTP network tests see `testcases/network/README.md`.
 | 'KCONFIG_PATH'        | The path to the kernel config file, (if not set, it tries
                           the usual paths '/boot/config-RELEASE' or '/proc/config.gz').
 | 'LTPROOT'             | Prefix for installed LTP, the default is '/opt/ltp'.
+                          Used for setting 'PATH' and other environment variables.
 | 'LTP_COLORIZE_OUTPUT' | Force colorized output behaviour. 'y' or '1': always colorize
                           'n' or '0': never colorize.
 | 'LTP_DEV'             | Path to the block device to be used
@@ -19,8 +20,10 @@ For running LTP network tests see `testcases/network/README.md`.
 | 'LTP_TIMEOUT_MUL'     | Multiply timeout, must be number >= 1 (> 1 is useful for
                           slow machines to avoid unexpected timeout).
                           Variable is also used in shell tests, but ceiled to int.
-| 'PATH'                | It's required to addjust path:
-                          `PATH="$PATH:$LTPROOT/testcases/bin"`
+| 'PATH'                | Testcases are installed into 'LTPROOT/testcases/bin',
+                          'PATH' is set by library API. You need to add it into
+                          'PATH' only if you use custom runner (not runltp neither
+                          [runltp-ng](https://github.com/metan-ucw/runltp-ng).
 | 'TMPDIR'              | Base directory for template directory (C: '.needs_tmpdir = 1'
                           and others, which imply it, shell: 'TST_NEEDS_TMPDIR=1').
 | 'TST_NO_CLEANUP'      | Disable running test cleanup (defined in 'TST_CLEANUP').
-- 
2.32.0

[LTP] [RFC PATCH 3/3] doc: Update LTPROOT and PATH environment variables

[Li Wang]

On Wed, Jun 16, 2021 at 12:33 AM Petr Vorel <pvorel@suse.cz> wrote:
>
> to address changes in two previous commits.
>
> Signed-off-by: Petr Vorel <pvorel@suse.cz>
> ---
>  README.md          | 2 +-
>  doc/user-guide.txt | 7 +++++--
>  2 files changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/README.md b/README.md
> index 703395c6b..b0ad47dfa 100644
> --- a/README.md
> +++ b/README.md
> @@ -162,7 +162,7 @@ $ testcases/bin/ioctl01 -h
>  Many require certain environment variables to be set
>
>  ```
> -$ LTPROOT=/opt/ltp PATH="$PATH:$LTPROOT/testcases/bin" testcases/bin/wc01.sh
> +$ LTPROOT=/opt/ltp testcases/bin/wc01.sh

I'm wondering does this really work? or, did I miss something?

Experiment in my kvm guest (with apply your patches):

# LTPROOT=/root/ltp-install wc01.sh
bash: wc01.sh: command not found...

# LTPROOT=/root/ltp-install testcases/bin/wc01.sh
-bash: testcases/bin/wc01.sh: No such file or directory

# LTPROOT=/root/ltp-install PATH="$PATH:$LTPROOT/testcases/bin" wc01.sh
wc01 1 TINFO: timeout per run is 0h 5m 0s
wc01 1 TPASS: wc passed with -c option.
wc01 2 TPASS: wc passed with --bytes option.
wc01 3 TPASS: wc passed with -l option.
wc01 4 TPASS: wc passed with --lines option.
wc01 5 TPASS: wc passed with -L option.
wc01 6 TPASS: wc passed with --max-line-length option.
wc01 7 TPASS: wc passed with -w option.
wc01 8 TPASS: wc passed with --words option.
wc01 9 TPASS: wc passed with -m option.
wc01 10 TPASS: wc passed with --chars option.
wc01 11 TPASS: wc passed with --help option.
wc01 12 TPASS: wc passed with --version option.

Summary:
passed   12
failed   0
broken   0
skipped  0
warnings 0

-- 
Regards,
Li Wang

[LTP] [RFC PATCH 3/3] doc: Update LTPROOT and PATH environment variables

[Petr Vorel]

Hi Li,

> > -$ LTPROOT=/opt/ltp PATH="$PATH:$LTPROOT/testcases/bin" testcases/bin/wc01.sh
> > +$ LTPROOT=/opt/ltp testcases/bin/wc01.sh

> I'm wondering does this really work? or, did I miss something?
Oops, I'm sorry to send broken patchset, this is obviously wrong.
We have to keep path set when calling script directly, because tst_test.sh would
be missing.

when in LPTROOT directory:
LTPROOT=/opt/ltp testcases/bin/wc01.sh
testcases/bin/wc01.sh: line 13: .: tst_test.sh: file not found

But we don't want to set path in each script nor load library as
. testcases/bin/tst_test.sh

Thus I guess setting PATH in LTP API doesn't make much sense if it works only
when using LTP runner (runltp{,-ng}, which BTW set path as well.
=> closing this.

> Experiment in my kvm guest (with apply your patches):

> # LTPROOT=/root/ltp-install wc01.sh
> bash: wc01.sh: command not found...
I would not expect this to be running, for this you obviously need to have set
PATH correctly.

> # LTPROOT=/root/ltp-install testcases/bin/wc01.sh
> -bash: testcases/bin/wc01.sh: No such file or directory
Nor this one.

Kind regards,
Petr

> # LTPROOT=/root/ltp-install PATH="$PATH:$LTPROOT/testcases/bin" wc01.sh
> wc01 1 TINFO: timeout per run is 0h 5m 0s
> wc01 1 TPASS: wc passed with -c option.
> wc01 2 TPASS: wc passed with --bytes option.
> wc01 3 TPASS: wc passed with -l option.
> wc01 4 TPASS: wc passed with --lines option.
> wc01 5 TPASS: wc passed with -L option.
> wc01 6 TPASS: wc passed with --max-line-length option.
> wc01 7 TPASS: wc passed with -w option.
> wc01 8 TPASS: wc passed with --words option.
> wc01 9 TPASS: wc passed with -m option.
> wc01 10 TPASS: wc passed with --chars option.
> wc01 11 TPASS: wc passed with --help option.
> wc01 12 TPASS: wc passed with --version option.

> Summary:
> passed   12
> failed   0
> broken   0
> skipped  0
> warnings 0

[LTP] [RFC PATCH 3/3] doc: Update LTPROOT and PATH environment variables

[Li Wang]

On Wed, Jun 16, 2021 at 6:02 PM Petr Vorel <pvorel@suse.cz> wrote:
>
> Hi Li,
>
> > > -$ LTPROOT=/opt/ltp PATH="$PATH:$LTPROOT/testcases/bin" testcases/bin/wc01.sh
> > > +$ LTPROOT=/opt/ltp testcases/bin/wc01.sh
>
> > I'm wondering does this really work? or, did I miss something?
> Oops, I'm sorry to send broken patchset, this is obviously wrong.
> We have to keep path set when calling script directly, because tst_test.sh would
> be missing.
>
> when in LPTROOT directory:
> LTPROOT=/opt/ltp testcases/bin/wc01.sh
> testcases/bin/wc01.sh: line 13: .: tst_test.sh: file not found
>
> But we don't want to set path in each script nor load library as
> . testcases/bin/tst_test.sh
>
> Thus I guess setting PATH in LTP API doesn't make much sense if it works only
> when using LTP runner (runltp{,-ng}, which BTW set path as well.

Exactly :).

-- 
Regards,
Li Wang