Re: [igt-dev] [PATCH i-g-t] runner: Use glib's regex utilities instead of POSIX ERE

Ser, Simon
2019-05-10T06:55:40Z

On Thu, 2019-05-09 at 20:06 -0400, Lyude wrote:
> From: Lyude Paul <lyude@redhat.com>
>
> POSIX ERE, while pretty standard is also very old and severely limited.
> In fact, it's so limited that Intel CI's own test blacklist,
> tests/intel-ci/blacklist.txt, doesn't even work with it due to the lack
> of support for backreferences.
>
> Since we're already using glib in other parts of igt, let's just start
> using glib's regular expression matching instead of the POSIX regex API.
> This gives us full perl compatible regular expressions and in turn, also
> gives us python compatible regular expressions to match piglit.

Side note: a more portable alternative would be PCRE.

> Signed-off-by: Lyude Paul <lyude@redhat.com>
> ---
> runner/job_list.c | 4 +---
> runner/meson.build | 2 +-
> runner/resultgen.c | 15 +++++++++------
> runner/settings.c | 26 ++++++++++----------------
> runner/settings.h | 4 ++--
> 5 files changed, 23 insertions(+), 28 deletions(-)
>
> diff --git a/runner/job_list.c b/runner/job_list.c
> index 941e2ee0..4a16742f 100644
> --- a/runner/job_list.c
> +++ b/runner/job_list.c
> @@ -17,10 +17,8 @@ static bool matches_any(const char *str, struct regex_list *list)
> size_t i;
>
> for (i = 0; i < list->size; i++) {
> - if (regexec(list->regexes[i], str,
> - (size_t)0, NULL, 0) == 0) {
> + if (g_regex_match(list->regexes[i], str, 0, NULL))
> return true;
> - }
> }
>
> return false;
> diff --git a/runner/meson.build b/runner/meson.build
> index de6e6f1c..1ffc95b4 100644
> --- a/runner/meson.build
> +++ b/runner/meson.build
> @@ -17,7 +17,7 @@ if _build_runner and jsonc.found()
>
> runnerlib = static_library('igt_runner', runnerlib_sources,
> include_directories : inc,
> - dependencies : jsonc)
> + dependencies : [jsonc, glib])
>
> runner = executable('igt_runner', runner_sources,
> link_with : runnerlib,
> diff --git a/runner/resultgen.c b/runner/resultgen.c
> index d9702a19..2b7d26d5 100644
> --- a/runner/resultgen.c
> +++ b/runner/resultgen.c
> @@ -499,14 +499,17 @@ static const char igt_dmesg_whitelist[] =
> static const char igt_piglit_style_dmesg_blacklist[] =
> "(\\[drm:|drm_|intel_|i915_)";
>
> -static bool init_regex_whitelist(struct settings* settings, regex_t* re)
> +static bool init_regex_whitelist(struct settings* settings, GRegex **re)
> {
> + GError *err = NULL;
> const char *regex = settings->piglit_style_dmesg ?
> igt_piglit_style_dmesg_blacklist :
> igt_dmesg_whitelist;
>
> - if (regcomp(re, regex, REG_EXTENDED | REG_NOSUB) != 0) {
> + *re = g_regex_new(regex, G_REGEX_OPTIMIZE, 0, &err);
> + if (err) {
> fprintf(stderr, "Cannot compile dmesg regexp\n");
> + g_error_free(err);
> return false;
> }
>
> @@ -630,7 +633,7 @@ static bool fill_from_dmesg(int fd,
> char piglit_name[256];
> ssize_t read;
> size_t i;
> - regex_t re;
> + GRegex *re;
>
> if (!f) {
> return false;
> @@ -671,12 +674,12 @@ static bool fill_from_dmesg(int fd,
>
> if (settings->piglit_style_dmesg) {
> if ((flags & 0x07) <= settings->dmesg_warn_level && continuation != 'c' &&
> - regexec(&re, message, (size_t)0, NULL, 0) != REG_NOMATCH) {
> + g_regex_match(re, message, 0, NULL)) {
> append_line(&warnings, &warningslen, formatted);
> }
> } else {
> if ((flags & 0x07) <= settings->dmesg_warn_level && continuation != 'c' &&
> - regexec(&re, message, (size_t)0, NULL, 0) == REG_NOMATCH) {
> + !g_regex_match(re, message, 0, NULL)) {
> append_line(&warnings, &warningslen, formatted);
> }
> }
> @@ -715,7 +718,7 @@ static bool fill_from_dmesg(int fd,
>
> free(dmesg);
> free(warnings);
> - regfree(&re);
> + g_regex_unref(re);
> fclose(f);
> return true;
> }
> diff --git a/runner/settings.c b/runner/settings.c
> index 25bcf531..ad38ae8d 100644
> --- a/runner/settings.c
> +++ b/runner/settings.c
> @@ -187,23 +187,18 @@ static void usage(const char *extra_message, FILE *f)
>
> static bool add_regex(struct regex_list *list, char *new)
> {
> - regex_t *regex;
> - size_t buflen;
> - char *buf;
> - int s;
> -
> - regex = malloc(sizeof(*regex));
> -
> - if ((s = regcomp(regex, new,
> - REG_EXTENDED | REG_NOSUB)) != 0) {
> - buflen = regerror(s, regex, NULL, 0);
> - buf = malloc(buflen);
> - regerror(s, regex, buf, buflen);
> + GRegex *regex;
> + GError *error = NULL;
> +
> + regex = g_regex_new(new, G_REGEX_OPTIMIZE, 0, &error);
> + if (error) {
> + char *buf = malloc(snprintf(NULL, 0, "Invalid regex '%s': %s", new, error->message) + 1);
> +
> + sprintf(buf, "Invalid regex '%s': %s", new, error->message);
> usage(buf, stderr);
>
> free(buf);
> - regfree(regex);
> - free(regex);
> + g_error_free(error);
> return false;
> }
>
> @@ -224,8 +219,7 @@ static void free_regexes(struct regex_list *regexes)
>
> for (i = 0; i < regexes->size; i++) {
> free(regexes->regex_strings[i]);
> - regfree(regexes->regexes[i]);
> - free(regexes->regexes[i]);
> + g_regex_unref(regexes->regexes[i]);
> }
> free(regexes->regex_strings);
> free(regexes->regexes);
> diff --git a/runner/settings.h b/runner/settings.h
> index 672a3af8..0a1ee08d 100644
> --- a/runner/settings.h
> +++ b/runner/settings.h
> @@ -4,8 +4,8 @@
> #include <stdbool.h>
> #include <stddef.h>
> #include <sys/types.h>
> -#include <regex.h>
> #include <stdio.h>
> +#include <glib.h>
>
> enum {
> LOG_LEVEL_NORMAL = 0,
> @@ -21,7 +21,7 @@ _Static_assert(ABORT_ALL == (ABORT_TAINT | ABORT_LOCKDEP), "ABORT_ALL must be al
>
> struct regex_list {
> char **regex_strings;
> - regex_t** regexes;
> + GRegex **regexes;
> size_t size;
> };