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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAEKGpziZTKxbdC7pER4bVV=_2Bm5apyFa_DdabkQPwQVQMKzkA@mail.gmail.com>
Date:   Wed, 18 Nov 2020 11:44:58 +0900
From:   "Daniel T. Lee" <danieltimlee@...il.com>
To:     Martin KaFai Lau <kafai@...com>
Cc:     Daniel Borkmann <daniel@...earbox.net>,
        Alexei Starovoitov <ast@...nel.org>,
        Andrii Nakryiko <andrii@...nel.org>, brakmo <brakmo@...com>,
        Jesper Dangaard Brouer <brouer@...hat.com>,
        Andrii Nakryiko <andrii.nakryiko@...il.com>,
        Lorenzo Bianconi <lorenzo@...nel.org>,
        David Ahern <dsa@...ulusnetworks.com>,
        Yonghong Song <yhs@...com>,
        Toke Høiland-Jørgensen <toke@...hat.com>,
        Ira Weiny <ira.weiny@...el.com>, Thomas Graf <tgraf@...g.ch>,
        Jakub Kicinski <kuba@...nel.org>,
        John Fastabend <john.fastabend@...il.com>,
        bpf <bpf@...r.kernel.org>, netdev <netdev@...r.kernel.org>,
        Xdp <xdp-newbies@...r.kernel.org>
Subject: 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 <kafai@...com> 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 <danieltimlee@...il.com>
> >
> > ---
> >  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