Re: [RFC bpf-next v1 5/8] selftests/bpf: no need to track next_match_pos in struct test_loader

Andrii Nakryiko
2024-07-02T21:18:30Z

On Tue, Jul 2, 2024 at 2:05 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> On Mon, 2024-07-01 at 17:41 -0700, Andrii Nakryiko wrote:
>
> [...]
>
> > > static void emit_verifier_log(const char *log_buf, bool force)
> > > @@ -450,23 +449,23 @@ static void validate_case(struct test_loader *tester,
> > > struct bpf_program *prog,
> > > int load_err)
> > > {
> > > - int i, j, err;
> > > - char *match;
> > > regmatch_t reg_match[1];
> > > + const char *match;
> > > + const char *log = tester->log_buf;
> > > + int i, j, err;
> > >
> > > for (i = 0; i < subspec->expect_msg_cnt; i++) {
> > > struct expect_msg *msg = &subspec->expect_msgs[i];
> > >
> > > if (msg->substr) {
> > > - match = strstr(tester->log_buf + tester->next_match_pos, msg->substr);
> > > + match = strstr(log, msg->substr);
> > > if (match)
> > > - tester->next_match_pos = match - tester->log_buf + strlen(msg->substr);
> > > + log += strlen(msg->substr);
> > > } else {
> > > - err = regexec(&msg->regex,
> > > - tester->log_buf + tester->next_match_pos, 1, reg_match, 0);
> > > + err = regexec(&msg->regex, log, 1, reg_match, 0);
> > > if (err == 0) {
> > > - match = tester->log_buf + tester->next_match_pos + reg_match[0].rm_so;
> > > - tester->next_match_pos += reg_match[0].rm_eo;
> > > + match = log + reg_match[0].rm_so;
> > > + log += reg_match[0].rm_eo;
> >
> > invert and simplify:
> >
> > log += reg_match[0].rm_eo;
> > match = log;
> >
> > ?
>
> The 'match' is at 'log + rm_so' (start offset).
> The 'log' is at 'log + rm_eo' (end offset).
>

oh... yeah... never mind... */me retreats*

> The brilliance of standard library naming.
>
> >
> > > } else {
> > > match = NULL;
> > > }
> >
> > how about we move this to the beginning of iteration (before `if
> > (msg->substr)`) and so we'll assume the match is NULL on regexec
> > failing?
>
> Ok, but this would require explicit match re-initialization to NULL at
> each iteration.

yes, which also makes it clear that we don't carry over match in
between iterations (we can move `const char *match` inside the for
loop to make it even clearer)

>
> …