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

[Daniel Borkmann <daniel@iogearbox.net>]

On 8/16/23 4:23 PM, Yafang Shao wrote:
> On Wed, Aug 16, 2023 at 5:56 PM Daniel Borkmann <daniel@iogearbox.net> 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@gmail.com>
>> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
>> Reviewed-by: Quentin Monnet <quentin@isovalent.com>
> 
> 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