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@xxxxxxxxxx>
>
> Ack with a nit below.
>
> Acked-by: Yonghong Song <yonghong.song@xxxxxxxxx>
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;
> > +}
> > +
> [...]
