[RFC,1/2] tst_test: Add test multiplex function
diff mbox series

Message ID 20190306152430.25219-1-chrubis@suse.cz
State Superseded
Delegated to: Petr Vorel
Headers show
Series
  • [RFC,1/2] tst_test: Add test multiplex function
Related show

Commit Message

Cyril Hrubis March 6, 2019, 3:24 p.m. UTC
The test multiplex function is intended for running the test function in
a loop for a different settings. The indended purpose is to run tests
for both libc wrapper and raw syscall as well as for different syscall
variants.

The commit itself adds a test_multiplex() function into the tst_test
structure, if set this function is called in a loop before each test
iteration and is responsible for changing the test variant into next
one. The iterations continue until the function returns zero.

Signed-off-by: Cyril Hrubis <chrubis@suse.cz>
CC: Mark Salyzyn <salyzyn@android.com>
CC: Steve Muckle <smuckle@google.com>
CC: Jan Stancek <jstancek@redhat.com>
---
 include/tst_test.h | 14 ++++++++++++++
 lib/tst_test.c     | 11 +++++++++--
 2 files changed, 23 insertions(+), 2 deletions(-)

Comments

Jan Stancek March 6, 2019, 4:53 p.m. UTC | #1
----- Original Message -----
> The test multiplex function is intended for running the test function in
> a loop for a different settings. The indended purpose is to run tests
> for both libc wrapper and raw syscall as well as for different syscall
> variants.
> 
> The commit itself adds a test_multiplex() function into the tst_test
> structure, if set this function is called in a loop before each test
> iteration and is responsible for changing the test variant into next
> one. The iterations continue until the function returns zero.

Hi,

on first look this looks like a workaround, because we have locked
ourselves out of .test function for timer tests.

Is there some way we could allow .test again for times tests?
Something user could override, provided he then runs the library
function that iterates over all usec values.

(sorry for pseudo-code)
// test
void my_test(int n)
{
  select_func = n;
  test_timer_test();
}

// library
void test_timer_test()
{
  for (i = 0; i < ARRAY_SIZE(tcases); i++)
    timer_test_fn(i);
}

Regards,
Jan

> 
> Signed-off-by: Cyril Hrubis <chrubis@suse.cz>
> CC: Mark Salyzyn <salyzyn@android.com>
> CC: Steve Muckle <smuckle@google.com>
> CC: Jan Stancek <jstancek@redhat.com>
> ---
>  include/tst_test.h | 14 ++++++++++++++
>  lib/tst_test.c     | 11 +++++++++--
>  2 files changed, 23 insertions(+), 2 deletions(-)
> 
> diff --git a/include/tst_test.h b/include/tst_test.h
> index da41f776d..b747b24d5 100644
> --- a/include/tst_test.h
> +++ b/include/tst_test.h
> @@ -146,6 +146,20 @@ struct tst_test {
>  	 */
>  	int all_filesystems:1;
>  
> +	/*
> +	 * If set this function is used to mutiplex between different test
> +	 * variants.
> +	 *
> +	 * Multiplexers are the way how to run the same test for different
> +	 * settings. The intended use is to test different syscall
> +	 * wrappers/variants but the API is generic and does not limit the
> +	 * usage in any way.
> +	 *
> +	 * Each time the function is called it switches to next test variant,
> +	 * returns zero if all variants has been iterated over.
> +	 */
> +	int (*test_multiplex)(void);
> +
>  	/* Minimal device size in megabytes */
>  	unsigned int dev_min_size;
>  
> diff --git a/lib/tst_test.c b/lib/tst_test.c
> index 7dd890b8d..6b033c879 100644
> --- a/lib/tst_test.c
> +++ b/lib/tst_test.c
> @@ -1009,8 +1009,15 @@ static void testrun(void)
>  		if (!cont)
>  			break;
>  
> -		run_tests();
> -		heartbeat();
> +		if (tst_test->test_multiplex) {
> +			while (tst_test->test_multiplex()) {
> +				run_tests();
> +				heartbeat();
> +			}
> +		} else {
> +			run_tests();
> +			heartbeat();
> +		}
>  	}
>  
>  	do_test_cleanup();
> --
> 2.19.2
> 
>
Cyril Hrubis March 6, 2019, 5 p.m. UTC | #2
Hi!
> on first look this looks like a workaround, because we have locked
> ourselves out of .test function for timer tests.

I do not follow you here, can you elaborate?

This patch has nothing to do with timer tests, it just allows whatever
the test does to be done several times with a hook to change some
settings prior to each iteration.
Jan Stancek March 6, 2019, 5:35 p.m. UTC | #3
----- Original Message -----
> Hi!
> > on first look this looks like a workaround, because we have locked
> > ourselves out of .test function for timer tests.
> 
> I do not follow you here, can you elaborate?

If this wasn't timer test, I'd ask why don't we use existing .test and .tcnt,
your test() func can be called with a parameter, so you could change
the code to choose correct syscall/glibc func based on value of that parameter.

For normal tests, this looks almost and .test/.tcnt functionality,
except test count can be also dynamic.

static int tcase = -1;

static void test(void)
{
   switch (tcase) {
   }
}

static int select_mpx(void)
{
    tcase++;
    if (tcase == 5)
      return 0;
    return 1;
}

static struct tst_test test = {
    .test_multiplex = select_mpx,
    .test_all = test,
}

> 
> This patch has nothing to do with timer tests, it just allows whatever
> the test does to be done several times with a hook to change some
> settings prior to each iteration.
> 
> --
> Cyril Hrubis
> chrubis@suse.cz
>
Cyril Hrubis March 6, 2019, 6:28 p.m. UTC | #4
Hi!
> > > on first look this looks like a workaround, because we have locked
> > > ourselves out of .test function for timer tests.
> > 
> > I do not follow you here, can you elaborate?
> 
> If this wasn't timer test, I'd ask why don't we use existing .test and .tcnt,
> your test() func can be called with a parameter, so you could change
> the code to choose correct syscall/glibc func based on value of that parameter.
> 
> For normal tests, this looks almost and .test/.tcnt functionality,
> except test count can be also dynamic.

Well so does the .all_filesystems flag, however I see these concepts to
be orthogonal to what the actual test does, which is the reason I want
them hooked up in the library rather than to be part of the testcase
code.
Jan Stancek March 6, 2019, 7:20 p.m. UTC | #5
----- Original Message -----
> Hi!
> > > > on first look this looks like a workaround, because we have locked
> > > > ourselves out of .test function for timer tests.
> > > 
> > > I do not follow you here, can you elaborate?
> > 
> > If this wasn't timer test, I'd ask why don't we use existing .test and
> > .tcnt,
> > your test() func can be called with a parameter, so you could change
> > the code to choose correct syscall/glibc func based on value of that
> > parameter.
> > 
> > For normal tests, this looks almost and .test/.tcnt functionality,
> > except test count can be also dynamic.
> 
> Well so does the .all_filesystems flag, however I see these concepts to
> be orthogonal to what the actual test does, which is the reason I want
> them hooked up in the library rather than to be part of the testcase
> code.

OK, so it's like another level on top of current test functions.

I probably would like 'variant' somewhere in name more, but overall
I'm not against the patch. Are you planning on adding some docs too?

Regards,
Jan
Steve Muckle March 6, 2019, 8:58 p.m. UTC | #6
On 03/06/2019 07:24 AM, Cyril Hrubis wrote:
> The test multiplex function is intended for running the test function in
> a loop for a different settings. The indended purpose is to run tests
> for both libc wrapper and raw syscall as well as for different syscall
> variants.
> 
> The commit itself adds a test_multiplex() function into the tst_test
> structure, if set this function is called in a loop before each test
> iteration and is responsible for changing the test variant into next
> one. The iterations continue until the function returns zero.
> 
> Signed-off-by: Cyril Hrubis <chrubis@suse.cz>
> CC: Mark Salyzyn <salyzyn@android.com>
> CC: Steve Muckle <smuckle@google.com>
> CC: Jan Stancek <jstancek@redhat.com>

Approach seems good to me.

Reviewed-by: Steve Muckle <smuckle@google.com>

> ---
>   include/tst_test.h | 14 ++++++++++++++
>   lib/tst_test.c     | 11 +++++++++--
>   2 files changed, 23 insertions(+), 2 deletions(-)
> 
> diff --git a/include/tst_test.h b/include/tst_test.h
> index da41f776d..b747b24d5 100644
> --- a/include/tst_test.h
> +++ b/include/tst_test.h
> @@ -146,6 +146,20 @@ struct tst_test {
>   	 */
>   	int all_filesystems:1;
>   
> +	/*
> +	 * If set this function is used to mutiplex between different test
> +	 * variants.
> +	 *
> +	 * Multiplexers are the way how to run the same test for different
> +	 * settings. The intended use is to test different syscall
> +	 * wrappers/variants but the API is generic and does not limit the
> +	 * usage in any way.
> +	 *
> +	 * Each time the function is called it switches to next test variant,
> +	 * returns zero if all variants has been iterated over.
> +	 */
> +	int (*test_multiplex)(void);
> +
>   	/* Minimal device size in megabytes */
>   	unsigned int dev_min_size;
>   
> diff --git a/lib/tst_test.c b/lib/tst_test.c
> index 7dd890b8d..6b033c879 100644
> --- a/lib/tst_test.c
> +++ b/lib/tst_test.c
> @@ -1009,8 +1009,15 @@ static void testrun(void)
>   		if (!cont)
>   			break;
>   
> -		run_tests();
> -		heartbeat();
> +		if (tst_test->test_multiplex) {
> +			while (tst_test->test_multiplex()) {
> +				run_tests();
> +				heartbeat();
> +			}
> +		} else {
> +			run_tests();
> +			heartbeat();
> +		}
>   	}
>   
>   	do_test_cleanup();
>
Cyril Hrubis March 7, 2019, 12:29 p.m. UTC | #7
Hi!
> > > If this wasn't timer test, I'd ask why don't we use existing .test and
> > > .tcnt,
> > > your test() func can be called with a parameter, so you could change
> > > the code to choose correct syscall/glibc func based on value of that
> > > parameter.
> > > 
> > > For normal tests, this looks almost and .test/.tcnt functionality,
> > > except test count can be also dynamic.
> > 
> > Well so does the .all_filesystems flag, however I see these concepts to
> > be orthogonal to what the actual test does, which is the reason I want
> > them hooked up in the library rather than to be part of the testcase
> > code.
> 
> OK, so it's like another level on top of current test functions.

Yes, the change to the library adds generic layer for that.

> I probably would like 'variant' somewhere in name more, but overall
> I'm not against the patch. Are you planning on adding some docs too?

I thought that calling it multiplex was descriptive enough name.


There is some summary in:

https://github.com/linux-test-project/ltp/issues/506

I guess that I can expand the text a bit and put it in the doc/
directory in the source tree along with the change to the test library.

Patch
diff mbox series

diff --git a/include/tst_test.h b/include/tst_test.h
index da41f776d..b747b24d5 100644
--- a/include/tst_test.h
+++ b/include/tst_test.h
@@ -146,6 +146,20 @@  struct tst_test {
 	 */
 	int all_filesystems:1;
 
+	/*
+	 * If set this function is used to mutiplex between different test
+	 * variants.
+	 *
+	 * Multiplexers are the way how to run the same test for different
+	 * settings. The intended use is to test different syscall
+	 * wrappers/variants but the API is generic and does not limit the
+	 * usage in any way.
+	 *
+	 * Each time the function is called it switches to next test variant,
+	 * returns zero if all variants has been iterated over.
+	 */
+	int (*test_multiplex)(void);
+
 	/* Minimal device size in megabytes */
 	unsigned int dev_min_size;
 
diff --git a/lib/tst_test.c b/lib/tst_test.c
index 7dd890b8d..6b033c879 100644
--- a/lib/tst_test.c
+++ b/lib/tst_test.c
@@ -1009,8 +1009,15 @@  static void testrun(void)
 		if (!cont)
 			break;
 
-		run_tests();
-		heartbeat();
+		if (tst_test->test_multiplex) {
+			while (tst_test->test_multiplex()) {
+				run_tests();
+				heartbeat();
+			}
+		} else {
+			run_tests();
+			heartbeat();
+		}
 	}
 
 	do_test_cleanup();