netdev - Re: [v3,4/4] tools: bpftool: add documentation for net attach/detach

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]

Date:   Fri, 9 Aug 2019 05:28:02 +0900
From:   "Daniel T. Lee" <danieltimlee@...il.com>
To:     Quentin Monnet <quentin.monnet@...ronome.com>
Cc:     Daniel Borkmann <daniel@...earbox.net>,
        Alexei Starovoitov <ast@...nel.org>,
        netdev <netdev@...r.kernel.org>
Subject: Re: [v3,4/4] tools: bpftool: add documentation for net attach/detach

On Fri, Aug 9, 2019 at 1:48 AM Quentin Monnet
<quentin.monnet@...ronome.com> wrote:
>
> 2019-08-07 11:25 UTC+0900 ~ Daniel T. Lee <danieltimlee@...il.com>
> > Since, new sub-command 'net attach/detach' has been added for
> > attaching XDP program on interface,
> > this commit documents usage and sample output of `net attach/detach`.
> >
> > Signed-off-by: Daniel T. Lee <danieltimlee@...il.com>
> > ---
> >  .../bpf/bpftool/Documentation/bpftool-net.rst | 51 +++++++++++++++++--
> >  1 file changed, 48 insertions(+), 3 deletions(-)
> >
> > diff --git a/tools/bpf/bpftool/Documentation/bpftool-net.rst b/tools/bpf/bpftool/Documentation/bpftool-net.rst
> > index d8e5237a2085..4ad1a380e186 100644
> > --- a/tools/bpf/bpftool/Documentation/bpftool-net.rst
> > +++ b/tools/bpf/bpftool/Documentation/bpftool-net.rst
> > @@ -15,17 +15,22 @@ SYNOPSIS
> >       *OPTIONS* := { [{ **-j** | **--json** }] [{ **-p** | **--pretty** }] }
> >
> >       *COMMANDS* :=
> > -     { **show** | **list** } [ **dev** name ] | **help**
> > +     { **show** | **list** | **attach** | **detach** | **help** }
> >
> >  NET COMMANDS
> >  ============
> >
> > -|    **bpftool** **net { show | list } [ dev name ]**
> > +|    **bpftool** **net { show | list }** [ **dev** *name* ]
> > +|    **bpftool** **net attach** *ATTACH_TYPE* *PROG* **dev** *name* [ **overwrite** ]
> > +|    **bpftool** **net detach** *ATTACH_TYPE* **dev** *name*
>
> Nit: Could we have "name" in capital letters (everywhere in the file),
> to make this file consistent with the formatting used for
> bpftool-prog.rst and bpftool-map.rst?
>

I'll update all "name" with capital "NAME" at next version of patch.

> >  |    **bpftool** **net help**
> > +|
> > +|    *PROG* := { **id** *PROG_ID* | **pinned** *FILE* | **tag** *PROG_TAG* }
> > +|    *ATTACH_TYPE* := { **xdp** | **xdpgeneric** | **xdpdrv** | **xdpoffload** }
> >
> >  DESCRIPTION
> >  ===========
> > -     **bpftool net { show | list } [ dev name ]**
> > +     **bpftool net { show | list }** [ **dev** *name* ]
> >                    List bpf program attachments in the kernel networking subsystem.
> >
> >                    Currently, only device driver xdp attachments and tc filter
> > @@ -47,6 +52,18 @@ DESCRIPTION
> >                    all bpf programs attached to non clsact qdiscs, and finally all
> >                    bpf programs attached to root and clsact qdisc.
> >
> > +     **bpftool** **net attach** *ATTACH_TYPE* *PROG* **dev** *name* [ **overwrite** ]
> > +                  Attach bpf program *PROG* to network interface *name* with
> > +                  type specified by *ATTACH_TYPE*. Previously attached bpf program
> > +                  can be replaced by the command used with **overwrite** option.
> > +                  Currently, *ATTACH_TYPE* only contains XDP programs.
>
> Other nit: "ATTACH_TYPE only contains XDP programs" sounds odd to me.
> Could we maybe phrase this something like: "Currently, only XDP-related
> modes are supported for ATTACH_TYPE"?
>
> Also, could you please provide a brief description of the different
> attach types? In particular, explaining what "xdp" alone stands for
> might be useful.
>

I'll replace the phrase and add brief description about the attach types.

> Thanks,
> Quentin
>
> > +
> > +     **bpftool** **net detach** *ATTACH_TYPE* **dev** *name*
> > +                  Detach bpf program attached to network interface *name* with
> > +                  type specified by *ATTACH_TYPE*. To detach bpf program, same
> > +                  *ATTACH_TYPE* previously used for attach must be specified.
> > +                  Currently, *ATTACH_TYPE* only contains XDP programs.

Thank you for taking your time for the review.