[LTP] [PATCH v2 0/3] Add support for debugging .all_filesystems

[LTP] [PATCH v2 0/3] Add support for debugging .all_filesystems

[Petr Vorel]

Hi all,

Changes v1->v2:
* 2 new commits (add tst_defaults.h, print variables on -h)
* rename variable to LTP_SINGLE_FS_TYPE

Kind regards,
Petr

Petr Vorel (3):
  lib: Introduce tst_defaults.h
  tst_test: Print environment variables on -h
  lib: Add support for debugging .all_filesystems

 doc/user-guide.txt           |  2 ++
 include/old/ltp_priv.h       | 13 +------------
 include/tst_defaults.h       | 21 +++++++++++++++++++++
 include/tst_private.h        |  1 +
 lib/tst_supported_fs_types.c |  9 +++++++++
 lib/tst_test.c               | 14 ++++++++++++++
 6 files changed, 48 insertions(+), 12 deletions(-)
 create mode 100644 include/tst_defaults.h

-- 
2.34.1


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

[LTP] [PATCH v2 1/3] lib: Introduce tst_defaults.h

[Petr Vorel]

Needed to reuse DEFAULT_FS_TYPE in next commit,
but put there also TEMPDIR, which is also variable used in both legacy
and new API.

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
new in v2

 include/old/ltp_priv.h | 13 +------------
 include/tst_defaults.h | 21 +++++++++++++++++++++
 include/tst_private.h  |  1 +
 3 files changed, 23 insertions(+), 12 deletions(-)
 create mode 100644 include/tst_defaults.h

diff --git a/include/old/ltp_priv.h b/include/old/ltp_priv.h
index 0552457e59..0a0ef70f33 100644
--- a/include/old/ltp_priv.h
+++ b/include/old/ltp_priv.h
@@ -23,18 +23,7 @@
 #define __LTP_PRIV_H__
 
 #include <stdarg.h>
-
-/*
- * This is the default temporary directory used by tst_tmpdir().
- *
- * This is used when TMPDIR env variable is not set.
- */
-#define TEMPDIR	"/tmp"
-
-/*
- * Default filesystem to be used for tests.
- */
-#define DEFAULT_FS_TYPE "ext2"
+#include "tst_defaults.h"
 
 /* environment variables for controlling  tst_res verbosity */
 #define TOUT_VERBOSE_S  "VERBOSE"	/* All test cases reported */
diff --git a/include/tst_defaults.h b/include/tst_defaults.h
new file mode 100644
index 0000000000..083427b7e2
--- /dev/null
+++ b/include/tst_defaults.h
@@ -0,0 +1,21 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2013 Cyril Hrubis <chrubis@suse.cz>
+ */
+
+#ifndef TST_DEFAULTS_H_
+#define TST_DEFAULTS_H_
+
+/*
+ * This is the default temporary directory used by tst_tmpdir().
+ *
+ * This is used when TMPDIR env variable is not set.
+ */
+#define TEMPDIR	"/tmp"
+
+/*
+ * Default filesystem to be used for tests.
+ */
+#define DEFAULT_FS_TYPE "ext2"
+
+#endif /* TST_DEFAULTS_H_ */
diff --git a/include/tst_private.h b/include/tst_private.h
index b02f91228e..6f4f39b151 100644
--- a/include/tst_private.h
+++ b/include/tst_private.h
@@ -11,6 +11,7 @@
 
 #include <stdio.h>
 #include <netdb.h>
+#include "tst_defaults.h"
 
 #define MAX_IPV4_PREFIX 32
 #define MAX_IPV6_PREFIX 128
-- 
2.34.1


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

[LTP] [PATCH v2 2/3] tst_test: Print environment variables on -h

[Petr Vorel]

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
new in v2

 lib/tst_test.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/lib/tst_test.c b/lib/tst_test.c
index 265df6543b..f92ff858e9 100644
--- a/lib/tst_test.c
+++ b/lib/tst_test.c
@@ -475,6 +475,19 @@ static void print_help(void)
 {
 	unsigned int i;
 
+	/* see doc/user-guide.txt, which lists also shell API variables */
+	fprintf(stderr, "Environment Variables\n");
+	fprintf(stderr, "---------------------\n");
+	fprintf(stderr, "KCONFIG_PATH         Specify kernel config file\n");
+	fprintf(stderr, "LTPROOT              Prefix for installed LTP, the default is /opt/ltp\n");
+	fprintf(stderr, "LTP_COLORIZE_OUTPUT  Force colorized output behaviour (y/1 always, n/0: never)\n");
+	fprintf(stderr, "LTP_DEV              Path to the block device to be used (for .needs_device)\n");
+	fprintf(stderr, "LTP_DEV_FS_TYPE      Filesystem used for testing (default: %s)\n", DEFAULT_FS_TYPE);
+	fprintf(stderr, "LTP_TIMEOUT_MUL      Multiply timeout (must be number >= 1)\n");
+	fprintf(stderr, "LTP_VIRT_OVERRIDE    Overrides virtual machine detection (values: \"\"|kvm|microsoft|xen|zvm\n");
+	fprintf(stderr, "TMPDIR               Base directory for template directory (for .needs_tmpdir, default: %s)\n", TEMPDIR);
+	fprintf(stderr, "\n");
+
 	fprintf(stderr, "Options\n");
 	fprintf(stderr, "-------\n");
 
-- 
2.34.1


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

Re: [LTP] [PATCH v2 2/3] tst_test: Print environment variables on -h

[Tim.Bird]

> -----Original Message-----
> From: ltp <ltp-bounces+tim.bird=sony.com@lists.linux.it> On Behalf Of Petr Vorel
> 
> Signed-off-by: Petr Vorel <pvorel@suse.cz>
> ---
> new in v2
> 
>  lib/tst_test.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/lib/tst_test.c b/lib/tst_test.c
> index 265df6543b..f92ff858e9 100644
> --- a/lib/tst_test.c
> +++ b/lib/tst_test.c
> @@ -475,6 +475,19 @@ static void print_help(void)
>  {
>  	unsigned int i;
> 
> +	/* see doc/user-guide.txt, which lists also shell API variables */
> +	fprintf(stderr, "Environment Variables\n");
> +	fprintf(stderr, "---------------------\n");
> +	fprintf(stderr, "KCONFIG_PATH         Specify kernel config file\n");
> +	fprintf(stderr, "LTPROOT              Prefix for installed LTP, the default is /opt/ltp\n");
> +	fprintf(stderr, "LTP_COLORIZE_OUTPUT  Force colorized output behaviour (y/1 always, n/0: never)\n");
> +	fprintf(stderr, "LTP_DEV              Path to the block device to be used (for .needs_device)\n");
> +	fprintf(stderr, "LTP_DEV_FS_TYPE      Filesystem used for testing (default: %s)\n", DEFAULT_FS_TYPE);
> +	fprintf(stderr, "LTP_TIMEOUT_MUL      Multiply timeout (must be number >= 1)\n");

I think this should this be: "Timeout multiplier (must be a number >=1, should be an integer)

> +	fprintf(stderr, "LTP_VIRT_OVERRIDE    Overrides virtual machine detection (values: \"\"|kvm|microsoft|xen|zvm\n");
> +	fprintf(stderr, "TMPDIR               Base directory for template directory (for .needs_tmpdir, default: %s)\n", TEMPDIR);
> +	fprintf(stderr, "\n");
> +
>  	fprintf(stderr, "Options\n");
>  	fprintf(stderr, "-------\n");
> 
> --
> 2.34.1

 -- Tim


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

Re: [LTP] [PATCH v2 2/3] tst_test: Print environment variables on -h

[Petr Vorel]

Hi Tim,

> > +	fprintf(stderr, "LTP_TIMEOUT_MUL      Multiply timeout (must be number >= 1)\n");

> I think this should this be: "Timeout multiplier (must be a number >=1, should be an integer)

Also makes sense, thanks!

Kind regards,
Petr

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

Re: [LTP] [PATCH v2 2/3] tst_test: Print environment variables on -h

[Petr Vorel]

> Hi Tim,

> > > +	fprintf(stderr, "LTP_TIMEOUT_MUL      Multiply timeout (must be number >= 1)\n");

> > I think this should this be: "Timeout multiplier (must be a number >=1, should be an integer)

> Also makes sense, thanks!
Although it does not have to be integer.
=> "Timeout multiplier (must be a number >=1"

For C API it's used any value:
./open01
tst_test.c:1409: TINFO: Timeout per run is 0h 05m 00s

LTP_TIMEOUT_MUL=1.15 ./open01
tst_test.c:1409: TINFO: Timeout per run is 0h 05m 45s

For shell API:
./zram02.sh
zram02 1 TINFO: timeout per run is 0h 5m 0s
zram02 1 TINFO: timeout per run is 0h 7m 30s

LTP_TIMEOUT_MUL=1.15 ./zram02.sh
zram02 1 TINFO: ceiling LTP_TIMEOUT_MUL to 2
zram02 1 TINFO: timeout per run is 0h 10m 0s
zram02 1 TINFO: timeout per run is 0h 15m 0s

Doc [1] explain it: Variable is also used in shell tests, but ceiled to int.

Kind regards,
Petr

[1] https://github.com/linux-test-project/ltp/wiki/User-Guidelines

> Kind regards,
> Petr

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

Re: [LTP] [PATCH v2 2/3] tst_test: Print environment variables on -h

[Tim.Bird]

> -----Original Message-----
> From: Petr Vorel <pvorel@suse.cz>
> 
> > Hi Tim,
> 
> > > > +	fprintf(stderr, "LTP_TIMEOUT_MUL      Multiply timeout (must be number >= 1)\n");
> 
> > > I think this should this be: "Timeout multiplier (must be a number >=1, should be an integer)
> 
> > Also makes sense, thanks!
> Although it does not have to be integer.
> => "Timeout multiplier (must be a number >=1"
> 
> For C API it's used any value:
> ./open01
> tst_test.c:1409: TINFO: Timeout per run is 0h 05m 00s
> 
> LTP_TIMEOUT_MUL=1.15 ./open01
> tst_test.c:1409: TINFO: Timeout per run is 0h 05m 45s
>
When I grepped the source code I saw the usage in testcases/lib/tst_test.sh,
but somehow missed the usage in lib/tst_test.c (sloppy on my part!)

Thanks for pointing this out.
 
> For shell API:
> ./zram02.sh
> zram02 1 TINFO: timeout per run is 0h 5m 0s
> zram02 1 TINFO: timeout per run is 0h 7m 30s
> 
> LTP_TIMEOUT_MUL=1.15 ./zram02.sh
> zram02 1 TINFO: ceiling LTP_TIMEOUT_MUL to 2
> zram02 1 TINFO: timeout per run is 0h 10m 0s
> zram02 1 TINFO: timeout per run is 0h 15m 0s
> 
> Doc [1] explain it: Variable is also used in shell tests, but ceiled to int.
> 
Would it be useful to add a bit of code to tst_test.sh to handle
floating point?  The shell construct of '$((timeout * LTP_TIMEOUT_MUL))'
can't handle floating point, but floating point could be done pretty easily
with a callout to awk or bc.

I'm willing to look at this and submit a patch.  But does the shell
test system try to avoid dependencies on other utility programs (like awk or bc)?

Maybe rounding timeouts up is preferred behavior anyway, so
this is not needed.  What do you think?

In any event, I retract my "should be an integer" suggestion. :-)

 -- Tim


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

Re: [LTP] [PATCH v2 2/3] tst_test: Print environment variables on -h

[Petr Vorel]

Hi Tim,
> > -----Original Message-----
> > From: Petr Vorel <pvorel@suse.cz>

> > > Hi Tim,

> > > > > +	fprintf(stderr, "LTP_TIMEOUT_MUL      Multiply timeout (must be number >= 1)\n");

> > > > I think this should this be: "Timeout multiplier (must be a number >=1, should be an integer)

> > > Also makes sense, thanks!
> > Although it does not have to be integer.
> > => "Timeout multiplier (must be a number >=1"

> > For C API it's used any value:
> > ./open01
> > tst_test.c:1409: TINFO: Timeout per run is 0h 05m 00s

> > LTP_TIMEOUT_MUL=1.15 ./open01
> > tst_test.c:1409: TINFO: Timeout per run is 0h 05m 45s

> When I grepped the source code I saw the usage in testcases/lib/tst_test.sh,
> but somehow missed the usage in lib/tst_test.c (sloppy on my part!)
FYI this help is for C API. We should print variables also in shell API,
but I wanted to keep this patchset small.

> Thanks for pointing this out.

> > For shell API:
> > ./zram02.sh
> > zram02 1 TINFO: timeout per run is 0h 5m 0s
> > zram02 1 TINFO: timeout per run is 0h 7m 30s

> > LTP_TIMEOUT_MUL=1.15 ./zram02.sh
> > zram02 1 TINFO: ceiling LTP_TIMEOUT_MUL to 2
> > zram02 1 TINFO: timeout per run is 0h 10m 0s
> > zram02 1 TINFO: timeout per run is 0h 15m 0s

> > Doc [1] explain it: Variable is also used in shell tests, but ceiled to int.

> Would it be useful to add a bit of code to tst_test.sh to handle
> floating point?  The shell construct of '$((timeout * LTP_TIMEOUT_MUL))'
> can't handle floating point, but floating point could be done pretty easily
> with a callout to awk or bc.

> I'm willing to look at this and submit a patch.  But does the shell
> test system try to avoid dependencies on other utility programs (like awk or bc)?

> Maybe rounding timeouts up is preferred behavior anyway, so
> this is not needed.  What do you think?

[ Cc Joerg, Li and Cyril, who were involved in shell timeout ]
We spent quite a lot of time before we end-up with ceiling, I'd have to search
for the reasons. We also didn't think that it's a big problem to not being
precise on shell. But feel free to improve things. Just, generally we prefer to
wrote small C code instead adding new dependencies (bc etc) or trying to make
shell portable. Actually writing C parser would be few lines of code.

> In any event, I retract my "should be an integer" suggestion. :-)
You pointed out also wrong spelling, which I'm going to update.
Thanks a lot for your review!

Kind regards,
Petr

>  -- Tim


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

Re: [LTP] [PATCH v2 2/3] tst_test: Print environment variables on -h

[Li Wang]

[-- Attachment #1.1: Type: text/plain, Size: 1494 bytes --]

Hi Tim, Petr,

On Wed, Dec 15, 2021 at 2:54 AM Petr Vorel <pvorel@suse.cz> wrote:


>
> > Would it be useful to add a bit of code to tst_test.sh to handle
> > floating point?  The shell construct of '$((timeout * LTP_TIMEOUT_MUL))'
> > can't handle floating point, but floating point could be done pretty
> easily
> > with a callout to awk or bc.
>
> > I'm willing to look at this and submit a patch.  But does the shell
> > test system try to avoid dependencies on other utility programs (like
> awk or bc)?
>
> > Maybe rounding timeouts up is preferred behavior anyway, so
> > this is not needed.  What do you think?
>
> [ Cc Joerg, Li and Cyril, who were involved in shell timeout ]
> We spent quite a lot of time before we end-up with ceiling, I'd have to
> search
> for the reasons. We also didn't think that it's a big problem to not being
> precise on shell. But feel free to improve things. Just, generally we
> prefer to
>

Yeah, I agree with this.

To be honest, I personally have no thoughts to make the shell
to handle float point, and by now I don't see any person request
that, for most situations we just use an round up number
to satisfy ourselves.

But anyway, I have no objection to improving that. Feel
free to submit a patch :).



> wrote small C code instead adding new dependencies (bc etc) or trying to
> make
> shell portable. Actually writing C parser would be few lines of code.
>

+1

For patch itself:
Reviewed-by: Li Wang <liwang@redhat.com>

-- 
Regards,
Li Wang

[-- Attachment #1.2: Type: text/html, Size: 3045 bytes --]

[-- Attachment #2: Type: text/plain, Size: 60 bytes --]


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

Re: [LTP] [PATCH v2 2/3] tst_test: Print environment variables on -h

[Cyril Hrubis]

Hi!
> [ Cc Joerg, Li and Cyril, who were involved in shell timeout ]
> We spent quite a lot of time before we end-up with ceiling, I'd have to search
> for the reasons. We also didn't think that it's a big problem to not being
> precise on shell. But feel free to improve things. Just, generally we prefer to
> wrote small C code instead adding new dependencies (bc etc) or trying to make
> shell portable. Actually writing C parser would be few lines of code.

We even have tst_multiply_timeout() function in the C API that does all
the work. So this should be as simple as adding another C helper that
would take one number on commandline as a parameter and print the
result after calling the function.

-- 
Cyril Hrubis
chrubis@suse.cz

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

[LTP] [PATCH v2 3/3] lib: Add support for debugging .all_filesystems

[Petr Vorel]

with LTP_SINGLE_FS_TYPE environment variable tests only that
filesystem instead of all supported filesystems.

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
Changes v1->v2:
* rename variable to LTP_SINGLE_FS_TYPE
* print help in -h

 doc/user-guide.txt           | 2 ++
 lib/tst_supported_fs_types.c | 9 +++++++++
 lib/tst_test.c               | 1 +
 3 files changed, 12 insertions(+)

diff --git a/doc/user-guide.txt b/doc/user-guide.txt
index 6a9fb33005..244392a6a3 100644
--- a/doc/user-guide.txt
+++ b/doc/user-guide.txt
@@ -15,6 +15,8 @@ For running LTP network tests see `testcases/network/README.md`.
                           'n' or '0': never colorize.
 | 'LTP_DEV'             | Path to the block device to be used
                           (C: '.needs_device = 1', shell: 'TST_NEEDS_DEVICE=1').
+| 'LTP_SINGLE_FS_TYPE'  | Testing only specified filesystem instead all
+                          supported (for tests with '.all_filesystems').
 | 'LTP_DEV_FS_TYPE'     | Filesystem used for testing (default: 'ext2').
 | 'LTP_TIMEOUT_MUL'     | Multiply timeout, must be number >= 1 (> 1 is useful for
                           slow machines to avoid unexpected timeout).
diff --git a/lib/tst_supported_fs_types.c b/lib/tst_supported_fs_types.c
index fc072cadfd..23e5ce8775 100644
--- a/lib/tst_supported_fs_types.c
+++ b/lib/tst_supported_fs_types.c
@@ -139,8 +139,17 @@ const char **tst_get_supported_fs_types(const char *const *skiplist)
 	unsigned int i, j = 0;
 	int skip_fuse;
 	enum tst_fs_impl sup;
+	const char *only_fs;
 
 	skip_fuse = tst_fs_in_skiplist("fuse", skiplist);
+	only_fs = getenv("LTP_SINGLE_FS_TYPE");
+
+	if (only_fs) {
+		tst_res(TINFO, "WARNING: testing only %s", only_fs);
+		if (tst_fs_is_supported(only_fs))
+			fs_types[0] = only_fs;
+		return fs_types;
+	}
 
 	for (i = 0; fs_type_whitelist[i]; i++) {
 		if (tst_fs_in_skiplist(fs_type_whitelist[i], skiplist)) {
diff --git a/lib/tst_test.c b/lib/tst_test.c
index f92ff858e9..ce2b8239d7 100644
--- a/lib/tst_test.c
+++ b/lib/tst_test.c
@@ -483,6 +483,7 @@ static void print_help(void)
 	fprintf(stderr, "LTP_COLORIZE_OUTPUT  Force colorized output behaviour (y/1 always, n/0: never)\n");
 	fprintf(stderr, "LTP_DEV              Path to the block device to be used (for .needs_device)\n");
 	fprintf(stderr, "LTP_DEV_FS_TYPE      Filesystem used for testing (default: %s)\n", DEFAULT_FS_TYPE);
+	fprintf(stderr, "LTP_SINGLE_FS_TYPE   Testing only specified filesystem instead all supported (for .all_filesystems)\n");
 	fprintf(stderr, "LTP_TIMEOUT_MUL      Multiply timeout (must be number >= 1)\n");
 	fprintf(stderr, "LTP_VIRT_OVERRIDE    Overrides virtual machine detection (values: \"\"|kvm|microsoft|xen|zvm\n");
 	fprintf(stderr, "TMPDIR               Base directory for template directory (for .needs_tmpdir, default: %s)\n", TEMPDIR);
-- 
2.34.1


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

Re: [LTP] [PATCH v2 3/3] lib: Add support for debugging .all_filesystems

[Cyril Hrubis]

Hi!
> +| 'LTP_SINGLE_FS_TYPE'  | Testing only specified filesystem instead all
                                         ^   ^
					 |   "specifies"
					 there should be comma or dash here

> +                          supported (for tests with '.all_filesystems').
>  | 'LTP_DEV_FS_TYPE'     | Filesystem used for testing (default: 'ext2').
>  | 'LTP_TIMEOUT_MUL'     | Multiply timeout, must be number >= 1 (> 1 is useful for
>                            slow machines to avoid unexpected timeout).
> diff --git a/lib/tst_supported_fs_types.c b/lib/tst_supported_fs_types.c
> index fc072cadfd..23e5ce8775 100644
> --- a/lib/tst_supported_fs_types.c
> +++ b/lib/tst_supported_fs_types.c
> @@ -139,8 +139,17 @@ const char **tst_get_supported_fs_types(const char *const *skiplist)
>  	unsigned int i, j = 0;
>  	int skip_fuse;
>  	enum tst_fs_impl sup;
> +	const char *only_fs;
>  
>  	skip_fuse = tst_fs_in_skiplist("fuse", skiplist);
> +	only_fs = getenv("LTP_SINGLE_FS_TYPE");
> +
> +	if (only_fs) {
> +		tst_res(TINFO, "WARNING: testing only %s", only_fs);
> +		if (tst_fs_is_supported(only_fs))
> +			fs_types[0] = only_fs;
> +		return fs_types;
> +	}
>  
>  	for (i = 0; fs_type_whitelist[i]; i++) {
>  		if (tst_fs_in_skiplist(fs_type_whitelist[i], skiplist)) {
> diff --git a/lib/tst_test.c b/lib/tst_test.c
> index f92ff858e9..ce2b8239d7 100644
> --- a/lib/tst_test.c
> +++ b/lib/tst_test.c
> @@ -483,6 +483,7 @@ static void print_help(void)
>  	fprintf(stderr, "LTP_COLORIZE_OUTPUT  Force colorized output behaviour (y/1 always, n/0: never)\n");
>  	fprintf(stderr, "LTP_DEV              Path to the block device to be used (for .needs_device)\n");
>  	fprintf(stderr, "LTP_DEV_FS_TYPE      Filesystem used for testing (default: %s)\n", DEFAULT_FS_TYPE);
> +	fprintf(stderr, "LTP_SINGLE_FS_TYPE   Testing only specified filesystem instead all supported (for .all_filesystems)\n");
>  	fprintf(stderr, "LTP_TIMEOUT_MUL      Multiply timeout (must be number >= 1)\n");
>  	fprintf(stderr, "LTP_VIRT_OVERRIDE    Overrides virtual machine detection (values: \"\"|kvm|microsoft|xen|zvm\n");
>  	fprintf(stderr, "TMPDIR               Base directory for template directory (for .needs_tmpdir, default: %s)\n", TEMPDIR);

Other than that the rest looks fine, for the patchset:

Reviewed-by: Cyril Hrubis <chrubis@suse.cz>


-- 
Cyril Hrubis
chrubis@suse.cz

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

Re: [LTP] [PATCH v2 3/3] lib: Add support for debugging .all_filesystems

[Petr Vorel]

Hi Cyril, Li,

> Hi!
> > +| 'LTP_SINGLE_FS_TYPE'  | Testing only specified filesystem instead all
>                                          ^   ^
> 					 |   "specifies"
> 					 there should be comma or dash here

I meant "specific", i.e. Testing only specific filesystem instead all supported,
but "Testing only - specifies filesystem instead all supported" makes more
sense.

> Other than that the rest looks fine, for the patchset:
> Reviewed-by: Cyril Hrubis <chrubis@suse.cz>

Thanks! Waiting for Li and others for input before merging it.

Kind regards,
Petr

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

Re: [LTP] [PATCH v2 3/3] lib: Add support for debugging .all_filesystems

[Li Wang]

[-- Attachment #1.1: Type: text/plain, Size: 1085 bytes --]

On Wed, Dec 15, 2021 at 12:46 AM Petr Vorel <pvorel@suse.cz> wrote:

> Hi Cyril, Li,
>
> > Hi!
> > > +| 'LTP_SINGLE_FS_TYPE'  | Testing only specified filesystem instead
> all
> >                                          ^   ^
> >                                        |   "specifies"
> >                                        there should be comma or dash here
>
> I meant "specific", i.e. Testing only specific filesystem instead all
> supported,
>

+1



> but "Testing only - specifies filesystem instead all supported" makes more
> sense.
>

Hmm, I think it also makes sense to people who care about the
only one filesystem on their product. So this should _NOT_ be limit
for test/debug, because it can help to reduce testing time for specific
requirements.

Let's just provide a variable and leave the usage to users:).



>
> > Other than that the rest looks fine, for the patchset:
> > Reviewed-by: Cyril Hrubis <chrubis@suse.cz>
>
> Thanks! Waiting for Li and others for input before merging it.
>

Good work.
Reviewed-by: Li Wang <liwang@redhat.com>


-- 
Regards,
Li Wang

[-- Attachment #1.2: Type: text/html, Size: 2803 bytes --]

[-- Attachment #2: Type: text/plain, Size: 60 bytes --]


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

Re: [LTP] [PATCH v2 3/3] lib: Add support for debugging .all_filesystems

[Petr Vorel]

Hi Cyril, Li,

> > > > +| 'LTP_SINGLE_FS_TYPE'  | Testing only specified filesystem instead
> > all
> > >                                          ^   ^
> > >                                        |   "specifies"
> > >                                        there should be comma or dash here

> > I meant "specific", i.e. Testing only specific filesystem instead all
> > supported,


> +1



> > but "Testing only - specifies filesystem instead all supported" makes more
> > sense.


> Hmm, I think it also makes sense to people who care about the
> only one filesystem on their product. So this should _NOT_ be limit
> for test/debug, because it can help to reduce testing time for specific
> requirements.

> Let's just provide a variable and leave the usage to users:).

Right :).
@Cyril: agree with "Testing only specific filesystem instead all supported" ?

> > > Other than that the rest looks fine, for the patchset:
> > > Reviewed-by: Cyril Hrubis <chrubis@suse.cz>

> > Thanks! Waiting for Li and others for input before merging it.


> Good work.
> Reviewed-by: Li Wang <liwang@redhat.com>
Thanks!

Kind regards,
Petr

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

Re: [LTP] [PATCH v2 3/3] lib: Add support for debugging .all_filesystems

[Cyril Hrubis]

Hi!
> Hmm, I think it also makes sense to people who care about the
> only one filesystem on their product. So this should _NOT_ be limit
> for test/debug, because it can help to reduce testing time for specific
> requirements.

With that line of reasoning it should be just called LTP_FS_TYPES and
user should be able to specify a list...

-- 
Cyril Hrubis
chrubis@suse.cz

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

Re: [LTP] [PATCH v2 3/3] lib: Add support for debugging .all_filesystems

[Li Wang]

[-- Attachment #1.1: Type: text/plain, Size: 544 bytes --]

Hi Petr, Cyril

On Wed, Dec 15, 2021 at 5:29 PM Cyril Hrubis <chrubis@suse.cz> wrote:

> Hi!
> > Hmm, I think it also makes sense to people who care about the
> > only one filesystem on their product. So this should _NOT_ be limit
> > for test/debug, because it can help to reduce testing time for specific
> > requirements.
>
> With that line of reasoning it should be just called LTP_FS_TYPES and
> user should be able to specify a list...
>

Okay right, so just let it is, if there is new requirement we can add that.


-- 
Regards,
Li Wang

[-- Attachment #1.2: Type: text/html, Size: 1137 bytes --]

[-- Attachment #2: Type: text/plain, Size: 60 bytes --]


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

Re: [LTP] [PATCH v2 3/3] lib: Add support for debugging .all_filesystems

[Petr Vorel]

> Hi Petr, Cyril

> On Wed, Dec 15, 2021 at 5:29 PM Cyril Hrubis <chrubis@suse.cz> wrote:

> > Hi!
> > > Hmm, I think it also makes sense to people who care about the
> > > only one filesystem on their product. So this should _NOT_ be limit
> > > for test/debug, because it can help to reduce testing time for specific
> > > requirements.

> > With that line of reasoning it should be just called LTP_FS_TYPES and
> > user should be able to specify a list...


> Okay right, so just let it is, if there is new requirement we can add that.

OK, I'll merge it as is. Thanks a lot both!

Kind regards,
Petr

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