On ti, 2015-11-17 at 13:05 +0000, Thomas Wood wrote:
> On 16 November 2015 at 13:22, Joonas Lahtinen
> <joonas.lahtinen@xxxxxxxxxxxxxxx> wrote:
> > +static void _igt_kmsg_capture_dump(void)
> > +{
> >
<SNIP>
> > + //fputs(strchr(igt_kmsg_capture_dump_buf, ';') + 1,
> > stderr);
>
> This line can be removed if it is no longer needed.
>
Yeah, already removed from my v2.
>
> > @@ -1072,16 +1158,21 @@ void __igt_fail_assert(const char *domain,
> > const char *file, const int line,
> > */
> > void igt_exit(void)
> > {
> > + int status = IGT_EXIT_SUCCESS;
> > +
> > igt_exit_called = true;
> >
> > if (run_single_subtest && !run_single_subtest_found) {
> > igt_warn("Unknown subtest: %s\n",
> > run_single_subtest);
> > - exit(IGT_EXIT_INVALID);
> > + status = IGT_EXIT_INVALID;
> > + goto do_exit;
> > }
> >
> >
> > - if (igt_only_list_subtests())
> > - exit(IGT_EXIT_SUCCESS);
> > + if (igt_only_list_subtests()) {
> > + status = IGT_EXIT_SUCCESS;
> > + goto do_exit;
>
> It might be good to avoid allocating the kmsg buffer and file
> descriptor altogether if only listing subtests. There are also a few
> cases where igt_exit won't be called, such as when using the help
> options or when a test has no subtests and one of the subtest options
> is specified.
Yes, Chris suggested only opening the FD when a subtest starts and
closing after each one. Which comes to the point that the "single test
or 1 or more subtests" leads to quite a few different codepaths, and I
think it should be considered converting all the single tests to tests
with only one subtest, to make the code maintainable.
> > if (failed_one)
> > - exit(igt_exitcode);
> > + status = igt_exitcode;
> > else if (succeeded_one)
> > - exit(IGT_EXIT_SUCCESS);
> > + status = IGT_EXIT_SUCCESS;
> > else
> > - exit(IGT_EXIT_SKIP);
> > + status = IGT_EXIT_SKIP;
> > +do_exit:
> > + if (igt_kmsg_capture_fd != -1)
> > + close(igt_kmsg_capture_fd);
> > + if (igt_kmsg_capture_dump_buf != NULL)
> > + free(igt_kmsg_capture_dump_buf);
>
> Since this always needs to be run, would it be simpler to move it to
> the top of this function?
>
True, originally I did the dumping in this function so the position is
kind of leftover from there.
>
> > +
> > + exit(status);
> > }
> >
> > /* fork support code */
> > diff --git a/tests/Makefile.sources b/tests/Makefile.sources
> > index 8fb2de8..25f0c4a 100644
> > --- a/tests/Makefile.sources
> > +++ b/tests/Makefile.sources
> > @@ -62,6 +62,7 @@ TESTS_progs_M = \
> > gem_tiled_partial_pwrite_pread \
> > gem_userptr_blits \
> > gem_write_read_ring_switch \
> > + igt_capture \
> > kms_addfb_basic \
> > kms_atomic \
> > kms_cursor_crc \
> > diff --git a/tests/igt_capture.c b/tests/igt_capture.c
> > new file mode 100644
> > index 0000000..fd008d0
> > --- /dev/null
> > +++ b/tests/igt_capture.c
>
> Since this is an infrastructure test, it would be better placed in
> lib/tests. The binary will also need adding to .gitignore.
>
Ok, will move it. I do notice there already is igt_stats in there.
Regards, Joonas
>
> > @@ -0,0 +1,93 @@
> > +/*
> > + * Copyright © 2015 Intel Corporation
> > + *
> > + * Permission is hereby granted, free of charge, to any person
> > obtaining a
> > + * copy of this software and associated documentation files (the
> > "Software"),
> > + * to deal in the Software without restriction, including without
> > limitation
> > + * the rights to use, copy, modify, merge, publish, distribute,
> > sublicense,
> > + * and/or sell copies of the Software, and to permit persons to
> > whom the
> > + * Software is furnished to do so, subject to the following
> > conditions:
> > + *
> > + * The above copyright notice and this permission notice
> > (including the next
> > + * paragraph) shall be included in all copies or substantial
> > portions of the
> > + * Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> > EXPRESS OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> > MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO
> > EVENT SHALL
> > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
> > DAMAGES OR OTHER
> > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> > ARISING
> > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> > OTHER DEALINGS
> > + * IN THE SOFTWARE.
> > + *
> > + * Authors:
> > + * Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
> > + *
> > + */
> > +
> > +#include "igt.h"
> > +#include <unistd.h>
> > +#include <stdlib.h>
> > +#include <stdio.h>
> > +#include <string.h>
> > +#include <fcntl.h>
> > +#include <inttypes.h>
> > +#include <errno.h>
> > +#include <sys/stat.h>
> > +#include <sys/ioctl.h>
> > +
> > +static FILE* kmsg;
> > +
> > +static void
> > +test_kmsg(void)
> > +{
> > + fputs("TEST (KMSG)\n", kmsg);
> > + fflush(kmsg);
> > + igt_fail(IGT_EXIT_FAILURE);
> > +}
> > +
> > +static void
> > +test_warn(void)
> > +{
> > + igt_warn("TEST (WARN)\n");
> > + igt_fail(IGT_EXIT_FAILURE);
> > +}
> > +
> > +static void
> > +test_debug(void)
> > +{
> > + igt_debug("TEST (DEBUG)\n");
> > + igt_fail(IGT_EXIT_FAILURE);
> > +}
> > +
> > +static void
> > +test_combined(void)
> > +{
> > + igt_warn("TEST #1 (WARN)\n");
> > + fputs("TEST #1\n", kmsg);
> > + igt_warn("TEST #2 (WARN)\n");
> > + fputs("TEST #2\n", kmsg);
> > + fflush(kmsg);
> > + igt_fail(IGT_EXIT_FAILURE);
> > +}
> > +
> > +igt_main
> > +{
> > + igt_fixture {
> > + kmsg = fopen("/dev/kmsg", "w");
> > + igt_require(kmsg != NULL);
> > + }
> > +
> > + igt_subtest("kmsg")
> > + test_kmsg();
> > + igt_subtest("warn")
> > + test_warn();
> > + igt_subtest("debug")
> > + test_debug();
> > + igt_subtest("combined")
> > + test_combined();
> > +
> > + igt_fixture {
> > + fclose(kmsg);
> > + }
> > +}
> > --
> > 2.4.3
> >
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
