Some comments on Daniels' comments inline > -----Original Message----- > From: Daniel Vetter [mailto:daniel.vetter@xxxxxxxx] On Behalf Of Daniel > Vetter > Sent: Monday, July 07, 2014 5:10 PM > To: Gore, Tim > Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx > Subject: Re: [PATCH 2/2] intel-gpu-tools: Re-use igt_subtest_init > for simple tests > > On Fri, Jun 27, 2014 at 03:15:37PM +0100, tim.gore@xxxxxxxxx wrote: > > From: Tim Gore <tim.gore@xxxxxxxxx> > > > > igt_subtest_init mainly does stuff that we also want for simple/single > > tests, such as looking for --list-subtests and --help options and > > calling common_init. So just call this from igt_simple_init and then > > set tests_with_subtests to false. NOTE that this means that > > check_igt_exit is now registered as an exit handler for single tests, > > so need to make sure that ALL tests exit via igt_exit. > > > > Signed-off-by: Tim Gore <tim.gore@xxxxxxxxx> > > --- > > lib/igt_core.c | 32 ++++++++++++++++---------------- > > lib/igt_core.h | 4 ++-- > > tests/gem_gtt_hog.c | 2 +- > > tests/igt_simulation.c | 4 ++-- > > 4 files changed, 21 insertions(+), 21 deletions(-) > > > > diff --git a/lib/igt_core.c b/lib/igt_core.c index 7ac7ebe..aaeaa3b > > 100644 > > --- a/lib/igt_core.c > > +++ b/lib/igt_core.c > > @@ -458,13 +458,15 @@ void igt_subtest_init(int argc, char **argv) > > * #igt_simple_main block instead of stitching the tests's main() function > together > > * manually. > > */ > > -void igt_simple_init(void) > > +void igt_simple_init(int argc, char **argv) > > { > > - print_version(); > > - > > - oom_adjust_for_doom(); > > - > > - common_init(); > > + /* Use the same init function as is used with subtests - we want most > of its functionality */ > > + /* Note that this will install the igt_exit_handler so you need to exit > via igt_exit(), */ > > + /* Dont call exit() */ > > + igt_subtest_init(argc, argv); > > Not sure whether forcing the reuse of igt_subtest_init makes a lot of sense > since it requires a lot of follow-up changes all over. I'd just add a bit of > argparsing here with the required special cases, i.e. > - exit without doing anything for --list-subtests > - exit with 77 when anything is specified with --run-subtests > What I wanted to do here was start removing the distinction between single Tests and multiple test; this distinction seems somewhat artificial and doesn't seem to add much. igt_subtest_init does pretty much everything we want for single tests so I thought it made sense to re-use it. Perhaps the name should change, although this would lead to more knock on changes. Tim > Also I prefer if the piglit changes come together with this patch so that we > can roll it all out together. What piglet changes do you refer to? I have not made any piglit changes. Tim > -Daniel > > > + test_with_subtests = false; > > + if (list_subtests) > > + igt_exit(); > > } > > > > /* > > @@ -565,7 +567,7 @@ void igt_skip(const char *f, ...) > > assert(in_fixture); > > __igt_fixture_end(); > > } else { > > - exit(IGT_EXIT_SKIP); > > + igt_exit(); > > } > > } > > > > @@ -655,7 +657,7 @@ void igt_fail(int exitcode) > > __igt_fixture_end(); > > } > > > > - exit(exitcode); > > + igt_exit(); > > } > > } > > > > @@ -713,18 +715,16 @@ void igt_exit(void) > > if (igt_only_list_subtests()) > > exit(IGT_EXIT_SUCCESS); > > > > - if (!test_with_subtests) > > - exit(IGT_EXIT_SUCCESS); > > - > > - /* Calling this without calling one of the above is a failure */ > > - assert(skipped_one || succeeded_one || failed_one); > > + if (test_with_subtests) > > + /* Calling this without calling one of the above is a failure */ > > + assert(skipped_one || succeeded_one || failed_one); > > > > if (failed_one) > > exit(igt_exitcode); > > - else if (succeeded_one) > > - exit(IGT_EXIT_SUCCESS); > > - else > > + else if (skipped_one) > > exit(IGT_EXIT_SKIP); > > + else > > + exit(IGT_EXIT_SUCCESS); > > } > > > > /* fork support code */ > > diff --git a/lib/igt_core.h b/lib/igt_core.h index 9853e6b..cabbc3b > > 100644 > > --- a/lib/igt_core.h > > +++ b/lib/igt_core.h > > @@ -166,7 +166,7 @@ bool igt_only_list_subtests(void); > > * > > * Init for simple tests without subtests > > */ > > -void igt_simple_init(void); > > +void igt_simple_init(int argc, char **argv); > > > > /** > > * igt_simple_main: > > @@ -178,7 +178,7 @@ void igt_simple_init(void); #define > > igt_simple_main \ > > static void igt_tokencat(__real_main, __LINE__)(int argc, char > **argv); \ > > int main(int argc, char **argv) { \ > > - igt_simple_init(); \ > > + igt_simple_init(argc, argv); \ > > igt_tokencat(__real_main, __LINE__)(argc, argv); \ > > igt_exit(); \ > > } \ > > diff --git a/tests/gem_gtt_hog.c b/tests/gem_gtt_hog.c index > > 5d47540..f607ea0 100644 > > --- a/tests/gem_gtt_hog.c > > +++ b/tests/gem_gtt_hog.c > > @@ -150,7 +150,7 @@ static void run(data_t *data, int child) > > munmap(ptr, size); > > > > igt_assert(x == canary); > > - exit(0); > > + _exit(0); > > } > > > > igt_simple_main > > diff --git a/tests/igt_simulation.c b/tests/igt_simulation.c index > > 15cbe64..3048c9e 100644 > > --- a/tests/igt_simulation.c > > +++ b/tests/igt_simulation.c > > @@ -53,11 +53,11 @@ static int do_fork(void) > > assert(0); > > case 0: > > if (simple) { > > - igt_simple_init(); > > + igt_simple_init(1, argv_run); > > > > igt_skip_on_simulation(); > > > > - exit(0); > > + igt_exit(); > > } else { > > if (list_subtests) > > igt_subtest_init(2, argv_list); > > -- > > 1.9.2 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
