Re: [PATCH bpf-next 1/2] bpftool: Implement link show support for tcx

[Daniel Borkmann <daniel@xxxxxxxxxxxxx>]

On 8/16/23 4:23 PM, Yafang Shao wrote:
> On Wed, Aug 16, 2023 at 5:56 PM Daniel Borkmann <daniel@xxxxxxxxxxxxx> wrote:
> > Add support to dump tcx link information to bpftool. This adds a
> > common helper show_link_ifindex_{plain,json}() which can be reused
> > also for other link types. The plain text and json device output is
> > the same format as in bpftool net dump.
> >
> > Below shows an example link dump output along with a cgroup link
> > for comparison:
> >
> >    # bpftool link
> >    [...]
> >    10: cgroup  prog 1977
> >          cgroup_id 1  attach_type cgroup_inet6_post_bind
> >    [...]
> >    13: tcx  prog 2053
> >          ifindex enp5s0(3)  attach_type tcx_ingress
> >    14: tcx  prog 2080
> >          ifindex enp5s0(3)  attach_type tcx_egress
> >    [...]
> >
> > Equivalent json output:
> >
> >    # bpftool link --json
> >    [...]
> >    {
> >      "id": 10,
> >      "type": "cgroup",
> >      "prog_id": 1977,
> >      "cgroup_id": 1,
> >      "attach_type": "cgroup_inet6_post_bind"
> >    },
> >    [...]
> >    {
> >      "id": 13,
> >      "type": "tcx",
> >      "prog_id": 2053,
> >      "devname": "enp5s0",
> >      "ifindex": 3,
> >      "attach_type": "tcx_ingress"
> >    },
> >    {
> >      "id": 14,
> >      "type": "tcx",
> >      "prog_id": 2080,
> >      "devname": "enp5s0",
> >      "ifindex": 3,
> >      "attach_type": "tcx_egress"
> >    }
> >    [...]
> >
> > Suggested-by: Yafang Shao <laoar.shao@xxxxxxxxx>
> > Signed-off-by: Daniel Borkmann <daniel@xxxxxxxxxxxxx>
> > Reviewed-by: Quentin Monnet <quentin@xxxxxxxxxxxxx>
>
> Thanks for your work. This patch looks good to me.
> A minor nit below.
>
> > ---
> >   tools/bpf/bpftool/link.c | 37 +++++++++++++++++++++++++++++++++++++
> >   1 file changed, 37 insertions(+)
> >
> > diff --git a/tools/bpf/bpftool/link.c b/tools/bpf/bpftool/link.c
> > index 65a168df63bc..a3774594f154 100644
> > --- a/tools/bpf/bpftool/link.c
> > +++ b/tools/bpf/bpftool/link.c
> > @@ -150,6 +150,18 @@ static void show_link_attach_type_json(__u32 attach_type, json_writer_t *wtr)
> >                  jsonw_uint_field(wtr, "attach_type", attach_type);
> >   }
> >
> > +static void show_link_ifindex_json(__u32 ifindex, json_writer_t *wtr)
> > +{
> > +       char devname[IF_NAMESIZE] = "(unknown)";
> > +
> > +       if (ifindex)
> > +               if_indextoname(ifindex, devname);
> > +       else
> > +               snprintf(devname, sizeof(devname), "(detached)");
> > +       jsonw_string_field(wtr, "devname", devname);
> > +       jsonw_uint_field(wtr, "ifindex", ifindex);
> > +}
> > +
> >   static bool is_iter_map_target(const char *target_name)
> >   {
> >          return strcmp(target_name, "bpf_map_elem") == 0 ||
> > @@ -433,6 +445,10 @@ static int show_link_close_json(int fd, struct bpf_link_info *info)
> >          case BPF_LINK_TYPE_NETFILTER:
> >                  netfilter_dump_json(info, json_wtr);
> >                  break;
> > +       case BPF_LINK_TYPE_TCX:
> > +               show_link_ifindex_json(info->tcx.ifindex, json_wtr);
> > +               show_link_attach_type_json(info->tcx.attach_type, json_wtr);
> > +               break;
> >          case BPF_LINK_TYPE_STRUCT_OPS:
> >                  jsonw_uint_field(json_wtr, "map_id",
> >                                   info->struct_ops.map_id);
> > @@ -509,6 +525,22 @@ static void show_link_attach_type_plain(__u32 attach_type)
> >                  printf("attach_type %u  ", attach_type);
> >   }
> >
> > +static void show_link_ifindex_plain(__u32 ifindex)
> > +{
> > +       char devname[IF_NAMESIZE * 2] = "(unknown)";
> > +       char tmpname[IF_NAMESIZE];
> > +       char *ret = NULL;
> > +
> > +       if (ifindex)
> > +               ret = if_indextoname(ifindex, tmpname);
> > +       else
> > +               snprintf(devname, sizeof(devname), "(detached)");
> > +       if (ret)
> > +               snprintf(devname, sizeof(devname), "%s(%d)",
> > +                        tmpname, ifindex);
> > +       printf("ifindex %s  ", devname);
> > +}
>
> This function looks a little strange to me. What about the change below?
>
> static void show_link_ifindex_plain(__u32 ifindex)
> {
>          char devname[IF_NAMESIZE] = "(unknown)";
>
>          if (ifindex) {
>                  if_indextoname(ifindex, devname);
>                  printf("ifindex %s(%d)  ", devname, ifindex);
>          } else {
>                  printf("ifindex (detached)  ");
>          }
> }

Arguably, it's a corner case (and should never happen), but for the case
where the if_indextoname call fails, I only intended to print `ifindex (unknown)`
for the plain mode hence the check for if_indextoname success so that this
looks similar as `ifindex (detached)` situation.

Thanks,
Daniel