Re: [PATCH bpf-next v3 1/2] selftests/bpf: Support checks against a regular expression.

[Eduard Zingerman <eddyz87@gmail.com>]

On Tue, 2024-06-11 at 18:40 +0100, Cupertino Miranda wrote:
> Add support for __regex and __regex_unpriv macros to check the test
> execution output against a regular expression. This is similar to __msg
> and __msg_unpriv, however those expect full text matching.
> 
> Signed-off-by: Cupertino Miranda <cupertino.miranda@oracle.com>
> Cc: jose.marchesi@oracle.com
> Cc: david.faust@oracle.com
> Cc: Yonghong Song <yonghong.song@linux.dev>
> Cc: Eduard Zingerman <eddyz87@gmail.com>
> Cc: Andrii Nakryiko <andrii.nakryiko@gmail.com>
> ---

Overall looks good, could you please fix a few things noted below and respin?
Please add my ack on the respin.

Acked-by: Eduard Zingerman <eddyz87@gmail.com>

[...]

> @@ -89,6 +98,16 @@ void test_loader_fini(struct test_loader *tester)
>  
>  static void free_test_spec(struct test_spec *spec)
>  {
> +	int i;
> +
> +	/* Deallocate expect_msgs arrays. */
> +	for (i = 0; i < spec->priv.expect_msg_cnt; i++)
> +		if (spec->priv.expect_msgs && spec->priv.expect_msgs[i].regex_str)

I don't think situation when spec->priv.expect_msg_cnt > 0 and
spec->priv.expect_msgs == NULL is possible, conditions above and below
could be simplified to just "if (spec->[un]priv.expect_msgs[i].regex_str)"

> +			regfree(&spec->priv.expect_msgs[i].regex);
> +	for (i = 0; i < spec->unpriv.expect_msg_cnt; i++)
> +		if (spec->unpriv.expect_msgs && spec->unpriv.expect_msgs[i].regex_str)
> +			regfree(&spec->unpriv.expect_msgs[i].regex);
> +
>  	free(spec->priv.name);
>  	free(spec->unpriv.name);
>  	free(spec->priv.expect_msgs);
> @@ -100,17 +119,38 @@ static void free_test_spec(struct test_spec *spec)
>  	spec->unpriv.expect_msgs = NULL;
>  }
>  
> -static int push_msg(const char *msg, struct test_subspec *subspec)
> +static int push_msg(const char *substr, const char *regex_str, struct test_subspec *subspec)
>  {
>  	void *tmp;
> +	int regcomp_res;
> +	char error_msg[100];
> +	struct expect_msg *msg;
>  
> -	tmp = realloc(subspec->expect_msgs, (1 + subspec->expect_msg_cnt) * sizeof(void *));
> +	tmp = realloc(subspec->expect_msgs,
> +		      (1 + subspec->expect_msg_cnt) * sizeof(struct expect_msg));
>  	if (!tmp) {
>  		ASSERT_FAIL("failed to realloc memory for messages\n");
>  		return -ENOMEM;
>  	}
>  	subspec->expect_msgs = tmp;
> -	subspec->expect_msgs[subspec->expect_msg_cnt++] = msg;
> +	msg = &subspec->expect_msgs[subspec->expect_msg_cnt];
> +	subspec->expect_msg_cnt += 1;
> +
> +	if (substr) {
> +		msg->substr = substr;
> +		msg->regex_str = NULL;
> +	} else {
> +		msg->regex_str = regex_str;
> +		msg->substr = NULL;
> +		regcomp_res = regcomp(&msg->regex, regex_str, REG_EXTENDED|REG_NEWLINE);
> +		if (regcomp_res != 0) {
> +			regerror(regcomp_res, &msg->regex, error_msg, 100);
                                                                      ^^^^
Nit:                                                      sizeof(error_msg)

> +			fprintf(stderr, "Regexp compilation error in '%s': '%s'\n",
> +				regex_str, error_msg);
> +			ASSERT_FAIL("failed to compile regex\n");

Nit:                    these two calls could be combined as a single PRINT_FAIL().

> +			return -EINVAL;
> +		}
> +	}
>  
>  	return 0;
>  }

[...]

> @@ -337,16 +389,11 @@ static int parse_test_spec(struct test_loader *tester,
>  		}
>  
>  		if (!spec->unpriv.expect_msgs) {
> -			size_t sz = spec->priv.expect_msg_cnt * sizeof(void *);
> +			for (i = 0; i < spec->priv.expect_msg_cnt; i++) {
> +				struct expect_msg *msg = &spec->priv.expect_msgs[i];
>  
> -			spec->unpriv.expect_msgs = malloc(sz);
> -			if (!spec->unpriv.expect_msgs) {
> -				PRINT_FAIL("failed to allocate memory for unpriv.expect_msgs\n");
> -				err = -ENOMEM;
> -				goto cleanup;
> +				push_msg(msg->substr, msg->regex_str, &spec->unpriv);

Need to check push_msg() return value.

>  			}
> -			memcpy(spec->unpriv.expect_msgs, spec->priv.expect_msgs, sz);
> -			spec->unpriv.expect_msg_cnt = spec->priv.expect_msg_cnt;
>  		}
>  	}
>  

[...]