Re: [PATCHv2 bpf-next] selftests/bpf: Add read_trace_pipe_iter function

[Jiri Olsa <olsajiri@gmail.com>]

On Wed, Apr 10, 2024 at 05:09:18PM -0700, Yonghong Song wrote:
> 
> On 4/10/24 7:09 AM, Jiri Olsa wrote:
> > We have two printk tests reading trace_pipe in non blocking way,
> > with the very same code. Moving that in new read_trace_pipe_iter
> > function.
> > 
> > Current read_trace_pipe is used from sampless/bpf and needs to
> > do blocking read and printf of the trace_pipe data, using new
> > read_trace_pipe_iter to implement that.
> > 
> > Both printk tests do early checks for the number of found messages
> > and can bail earlier, but I did not find any speed difference w/o
> > that condition, so I did not complicate the change more for that.
> > 
> > Some of the samples/bpf programs use read_trace_pipe function,
> > so I kept that interface untouched. I did not see any issues with
> > affected samples/bpf programs other than there's slight change in
> > read_trace_pipe output. The current code uses puts that adds new
> > line after the printed string, so we would occasionally see extra
> > new line. With this patch we read output per lines, so there's no
> > need to use puts and we can use just printf instead without extra
> > new line.
> > 
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> 
> Ack with a nit below.
> 
> Acked-by: Yonghong Song <yonghong.song@linux.dev>

SNIP

> > @@ -56,21 +47,12 @@ void serial_test_trace_printk(void)
> >   		goto cleanup;
> >   	/* verify our search string is in the trace buffer */
> > -	while (getline(&buf, &buflen, fp) >= 0 || errno == EAGAIN) {
> > -		if (strstr(buf, SEARCHMSG) != NULL)
> > -			found++;
> > -		if (found == bss->trace_printk_ran)
> > -			break;
> 
> The above condition is not covered, but I think it is okay since the test
> will run in serial mode.

right, I made a note about that in the changelog:

> > Both printk tests do early checks for the number of found messages
> > and can bail earlier, but I did not find any speed difference w/o
> > that condition, so I did not complicate the change more for that.

I think it's fine

SNIP

> > +int read_trace_pipe_iter(void (*cb)(const char *str, void *data), void *data, int iter)
> > +{
> > +	size_t buflen, n;
> > +	char *buf = NULL;
> > +	FILE *fp = NULL;
> > +
> > +	if (access(TRACEFS_PIPE, F_OK) == 0)
> > +		fp = fopen(TRACEFS_PIPE, "r");
> > +	else
> > +		fp = fopen(DEBUGFS_PIPE, "r");
> > +	if (!fp)
> > +		return -1;
> > +
> > +	 /* We do not want to wait forever when iter is specified. */
> > +	if (iter)
> > +		fcntl(fileno(fp), F_SETFL, O_NONBLOCK);
> > +
> > +	while ((n = getline(&buf, &buflen, fp) >= 0) || errno == EAGAIN) {
> > +		if (n > 0)
> > +			cb(buf, data);
> > +		if (iter && !(--iter))
> 
> "if (iter-- == 1)" should also work. But your code works too.

ok, keeping the current one ;-)

thanks,
jirka

> 
> > +			break;
> > +	}
> > +
> > +	free(buf);
> > +	if (fp)
> > +		fclose(fp);
> > +	return 0;
> > +}
> > +
> [...]