On Wed, Aug 16, 2023 at 11:11 PM Daniel Borkmann <daniel@xxxxxxxxxxxxx> wrote:
>
> 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.
>
Fair enough.
Then fail free to add :
Acked-by: Yafang Shao <laoar.shao@xxxxxxxxx>
--
Regards
Yafang
