Re: [PATCH bpf-next 1/9] selftests: bpf: move tracing helpers to trace_helper

On Wed, Nov 18, 2020 at 10:19 AM Martin KaFai Lau <[email protected]> wrote:
>
> On Tue, Nov 17, 2020 at 02:56:36PM +0000, Daniel T. Lee wrote:
> > Under the samples/bpf directory, similar tracing helpers are
> > fragmented around. To keep consistent of tracing programs, this commit
> > moves the helper and define locations to increase the reuse of each
> > helper function.
> >
> > Signed-off-by: Daniel T. Lee <[email protected]>
> >
> > ---
> >  samples/bpf/Makefile                        |  2 +-
> >  samples/bpf/hbm.c                           | 51 ++++-----------------
> >  tools/testing/selftests/bpf/trace_helpers.c | 33 ++++++++++++-
> >  tools/testing/selftests/bpf/trace_helpers.h |  3 ++
> >  4 files changed, 45 insertions(+), 44 deletions(-)
> >
> > [...]
> >
> > -#define DEBUGFS "/sys/kernel/debug/tracing/"
> Is this change needed?

This macro can be used in other samples such as the 4th' patch of this
patchset, task_fd_query.

> > -
> >  #define MAX_SYMS 300000
> >  static struct ksym syms[MAX_SYMS];
> >  static int sym_cnt;
> > @@ -136,3 +134,34 @@ void read_trace_pipe(void)
> >               }
> >       }
> >  }
> > +
> > +void read_trace_pipe2(char *filename)
> > +{
> > +     int trace_fd;
> > +     FILE *outf;
> > +
> > +     trace_fd = open(DEBUGFS "trace_pipe", O_RDONLY, 0);
> > +     if (trace_fd < 0) {
> > +             printf("Error opening trace_pipe\n");
> > +             return;
> > +     }
> > +
> > +     outf = fopen(filename, "w");
> > +     if (!outf)
> > +             printf("Error creating %s\n", filename);
> > +
> > +     while (1) {
> > +             static char buf[4096];
> > +             ssize_t sz;
> > +
> > +             sz = read(trace_fd, buf, sizeof(buf) - 1);
> > +             if (sz > 0) {
> > +                     buf[sz] = 0;
> > +                     puts(buf);
> > +                     if (outf) {
> > +                             fprintf(outf, "%s\n", buf);
> > +                             fflush(outf);
> > +                     }
> > +             }
> > +     }
> It needs a fclose().
>
> IIUC, this function will never return.  I am not sure
> this is something that should be made available to selftests.

Actually, read_trace_pipe and read_trace_pipe2 are helpers that are
only used under samples directory. Since these helpers are not used
in selftests, What do you think about moving these helpers to
something like samples/bpf/trace_pipe.h?

Thanks for your time and effort for the review.

-- 
Best,
Daniel T. Lee