[PATCH i-g-t v1 1/4] runner/resultgen: Added dynamically ignored dmesg messages
Zbigniew Kempczyński
zbigniew.kempczynski at intel.com
Tue Jul 30 04:02:37 UTC 2024
On Thu, Jul 25, 2024 at 02:50:03PM +0200, Kamil Konieczny wrote:
> Some messages generated by driver are triggered by test itself
> and are not meant to rise error nor warn within runner, yet
> they should be catched in other circumstances so they are not
> suitable to be ignored permanently.
>
> Instead of hard-coding such situations check dmesg for info from
> a test and add such regex on a fly and then ignore next dmesg
> errors or warns which match it.
>
> Ignored regex will be removed after a new subtest or dynamic
> subtest starts or at end of current tests.
>
> Signed-off-by: Kamil Konieczny <kamil.konieczny at linux.intel.com>
> ---
> runner/output_strings.h | 8 +++++
> runner/resultgen.c | 75 ++++++++++++++++++++++++++++++++++++-----
> 2 files changed, 75 insertions(+), 8 deletions(-)
>
> diff --git a/runner/output_strings.h b/runner/output_strings.h
> index 892895ead..91ef5f2c3 100644
> --- a/runner/output_strings.h
> +++ b/runner/output_strings.h
> @@ -54,6 +54,14 @@ static const char STARTING_SUBTEST_DMESG[] = ": starting subtest ";
> */
> static const char STARTING_DYNAMIC_SUBTEST_DMESG[] = ": starting dynamic subtest ";
>
> +/*
> + * Output in dmesg when a test wants runner to dynamically ignore error or warn.
> + *
> + * Example:
> + * [IGT] add ignored dmesg regex: CRITICAL: Xe has declared device [0-9:.]* as wedged
> + */
> +static const char IGT_ADD_IGNORED_REGEX_DMESG[] = "add ignored dmesg regex: ";
> +
I wondered a bit about the design. You've decided to feed runner with
regexp instead of providing a file like ignored_dmesg.txt with format
igt at xe_wedged@.*:CRITICAL: Xe has declared device [0-9:.]* as wedged
I mean adding IGT_ADD_IGNORED_REGEX_DMESG binds tigthly test and runner
according to how some lines will be interpreted. And I'm not big fan
of such solution. But this is first time we want to change how some
lines are interpreted in the runner and there's only one test which
wants to do that. So conditionally I can accept this. But have some
nits which should be address before merging.
> /*
> * Output when a test process is executed.
> *
> diff --git a/runner/resultgen.c b/runner/resultgen.c
> index 63f5b26d7..b360d1416 100644
> --- a/runner/resultgen.c
> +++ b/runner/resultgen.c
> @@ -829,16 +829,13 @@ static const char igt_dmesg_whitelist[] =
> static const char igt_piglit_style_dmesg_blacklist[] =
> "(\\[drm:|drm_|intel_|i915_|\\[drm\\])";
>
> -static bool init_regex_whitelist(struct settings* settings, GRegex **re)
> +static bool init_dmesg_regex(GRegex **re, const char *regex, const char *msg)
> {
> GError *err = NULL;
> - const char *regex = settings->piglit_style_dmesg ?
> - igt_piglit_style_dmesg_blacklist :
> - igt_dmesg_whitelist;
>
> *re = g_regex_new(regex, G_REGEX_OPTIMIZE, 0, &err);
> if (err) {
> - fprintf(stderr, "Cannot compile dmesg regexp\n");
> + fprintf(stderr, "Cannot compile dmesg regexp for %s\n", msg);
> g_error_free(err);
> return false;
> }
> @@ -846,6 +843,56 @@ static bool init_regex_whitelist(struct settings* settings, GRegex **re)
> return true;
> }
>
> +static bool init_regex_whitelist(struct settings *settings, GRegex **re)
> +{
> + const char *regex = settings->piglit_style_dmesg ?
> + igt_piglit_style_dmesg_blacklist :
> + igt_dmesg_whitelist;
> + const char *what = settings->piglit_style_dmesg ?
> + "piglit style dmesg blocklist" :
> + "igt dmesg whitelist";
> +
> + return init_dmesg_regex(re, regex, what);
> +}
> +
> +static bool not_ignored(GRegex *re, const char *msg)
> +{
> + if (!re)
> + return true;
> +
> + return !g_regex_match(re, msg, 0, NULL);
> +}
> +
> +static void clean_regex(GRegex **re, char **src)
> +{
> + if (*src)
> + free(*src);
This is imo unnecessary. GRegex duplicates pattern string on
g_regex_new() and if you want have an access to this via
g_regex_get_pattern().
> +
> + if (*re)
> + g_regex_unref(*re);
> +
> + *re = NULL;
> + *src = NULL;
> +}
> +
> +static void add_ignored_regex(GRegex **re, char **dst, char *src)
> +{
> + char *s;
> +
> + s = strchr(src, '\n');
> + if (*s)
> + *s = 0;
> +
> + clean_regex(re, dst);
> + *dst = strdup(src);
Imo this is unnecessary.
> + if (*dst) {
> + init_dmesg_regex(re, *dst, "ignore match");
> + fprintf(stderr, "igt_resultgen: Added ignore regex '%s'\n", src);
> + } else {
> + fprintf(stderr, "igt_resultgen: Error: cannot dup ignore regex '%s'\n", src);
> + }
> +}
> +
> static bool parse_dmesg_line(char* line,
> unsigned *flags, unsigned long long *ts_usec,
> char *continuation, char **message)
> @@ -979,6 +1026,8 @@ static bool fill_from_dmesg(int fd,
> char dynamic_piglit_name[256];
> size_t i;
> GRegex *re;
> + GRegex *re_ignore; /* regex for dynamically ignored dmesg line */
> + char *str_ignore;
>
> if (!f) {
> return false;
> @@ -989,12 +1038,14 @@ static bool fill_from_dmesg(int fd,
> return false;
> }
>
> + re_ignore = NULL;
> + str_ignore = NULL;
> while (getline(&line, &linelen, f) > 0) {
> char *formatted;
> unsigned flags;
> unsigned long long ts_usec;
> char continuation;
> - char *message, *subtest, *dynamic_subtest;
> + char *message, *subtest, *dynamic_subtest, *ignore;
>
> if (!parse_dmesg_line(line, &flags, &ts_usec, &continuation, &message))
> continue;
> @@ -1024,6 +1075,7 @@ static bool fill_from_dmesg(int fd,
> subtest += strlen(STARTING_SUBTEST_DMESG);
> generate_piglit_name(binary, subtest, piglit_name, sizeof(piglit_name));
> current_test = get_or_create_json_object(tests, piglit_name);
> + clean_regex(&re_ignore, &str_ignore);
Necessity of calling clean_regex() is direct consequence of the design
you're feeding runner via magic lines from the test. I see one pros
of your solution. Additional file with regexps for ignored dmesg lines
likely would be kept in list, so each test will trigger walk over the
list. Your solution feeds runner exactly with what you want to ignore.
Please address my nit regarding unnecessary strdup and free and respin.
--
Zbigniew
> }
>
> if (current_test != NULL &&
> @@ -1041,18 +1093,24 @@ static bool fill_from_dmesg(int fd,
> dynamic_subtest += strlen(STARTING_DYNAMIC_SUBTEST_DMESG);
> generate_piglit_name_for_dynamic(piglit_name, dynamic_subtest, dynamic_piglit_name, sizeof(dynamic_piglit_name));
> current_dynamic_test = get_or_create_json_object(tests, dynamic_piglit_name);
> + clean_regex(&re_ignore, &str_ignore);
> }
>
> + if ((ignore = strstr(message, IGT_ADD_IGNORED_REGEX_DMESG)) != NULL)
> + add_ignored_regex(&re_ignore, &str_ignore, ignore + strlen(IGT_ADD_IGNORED_REGEX_DMESG));
> +
> if (settings->piglit_style_dmesg) {
> if ((flags & 0x07) <= settings->dmesg_warn_level && continuation != 'c' &&
> - g_regex_match(re, message, 0, NULL)) {
> + g_regex_match(re, message, 0, NULL) &&
> + not_ignored(re_ignore, message)) {
> append_line(&warnings, &warningslen, formatted);
> if (current_test != NULL)
> append_line(&dynamic_warnings, &dynamic_warnings_len, formatted);
> }
> } else {
> if ((flags & 0x07) <= settings->dmesg_warn_level && continuation != 'c' &&
> - !g_regex_match(re, message, 0, NULL)) {
> + !g_regex_match(re, message, 0, NULL) &&
> + not_ignored(re_ignore, message)) {
> append_line(&warnings, &warningslen, formatted);
> if (current_test != NULL)
> append_line(&dynamic_warnings, &dynamic_warnings_len, formatted);
> @@ -1099,6 +1157,7 @@ static bool fill_from_dmesg(int fd,
> free(dynamic_dmesg);
> free(warnings);
> free(dynamic_warnings);
> + clean_regex(&re_ignore, &str_ignore);
> g_regex_unref(re);
> fclose(f);
> return true;
> --
> 2.43.0
>
More information about the igt-dev
mailing list