Re: [igt-dev] [PATCH i-g-t 1/3] runner: Reinitialize compiled dmesg regexp each parsing session

Ser, Simon
2019-04-01T07:23:47Z

On Mon, 2019-04-01 at 09:46 +0300, Arkadiusz Hiler wrote:
> Which regexp gets compiled is settings specific, depending whether we
> run piglit-style or not.
>
> If it's optimized to be initialized only once and it is a global
> variable, it will be "stuck" in the mode we have selected with the
> first
> run, which may break tests.
>
> Let's remove this optimization and initialize it each time, as it takes
> less 0.002s on my hardware.
>
> Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
> ---
> runner/resultgen.c | 29 +++++++++++------------------
> 1 file changed, 11 insertions(+), 18 deletions(-)
>
> diff --git a/runner/resultgen.c b/runner/resultgen.c
> index 32b59d59..73fda64f 100644
> --- a/runner/resultgen.c
> +++ b/runner/resultgen.c
> @@ -499,27 +499,18 @@ static const char igt_dmesg_whitelist[] =
> static const char igt_piglit_style_dmesg_blacklist[] =
> "(\\[drm:|drm_|intel_|i915_)";
>
> -static regex_t re;
> -
> -static int init_regex_whitelist(struct settings *settings)
> +static bool init_regex_whitelist(struct settings *settings, regex_t* re)

Style nit: the * should be on the right

Otherwise, this is:

Reviewed-by: Simon Ser <simon.ser@intel.com>

> {
> - static int status = -1;
> + const char *regex = settings->piglit_style_dmesg ?
> + igt_piglit_style_dmesg_blacklist :
> + igt_dmesg_whitelist;
>
> - if (status == -1) {
> - const char *regex = settings->piglit_style_dmesg ?
> - igt_piglit_style_dmesg_blacklist :
> - igt_dmesg_whitelist;
> -
> - if (regcomp(&re, regex, REG_EXTENDED | REG_NOSUB) != 0)
> {
> - fprintf(stderr, "Cannot compile dmesg
> regexp\n");
> - status = 1;
> - return false;
> - }
> -
> - status = 0;
> + if (regcomp(re, regex, REG_EXTENDED | REG_NOSUB) != 0) {
> + fprintf(stderr, "Cannot compile dmesg regexp\n");
> + return false;
> }
>
> - return status;
> + return true;
> }
>
> static bool parse_dmesg_line(char* line,
> @@ -639,12 +630,13 @@ static bool fill_from_dmesg(int fd,
> char piglit_name[256];
> ssize_t read;
> size_t i;
> + regex_t re;
>
> if (!f) {
> return false;
> }
>
> - if (init_regex_whitelist(settings)) {
> + if (!init_regex_whitelist(settings, &re)) {
> fclose(f);
> return false;
> }
> @@ -723,6 +715,7 @@ static bool fill_from_dmesg(int fd,
>
> free(dmesg);
> free(warnings);
> + regfree(&re);
> fclose(f);
> return true;
> }