| Message ID | 20200531082846.2117903-8-jakub@cloudflare.com |
|---|---|
| State | Accepted |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | Link-based program attachment to network namespaces | expand |
On Sun, May 31, 2020 at 1:32 AM Jakub Sitnicki <jakub@cloudflare.com> wrote: > > Code for printing link attach_type is duplicated in a couple of places, and > likely will be duplicated for future link types as well. Create helpers to > prevent duplication. > > Suggested-by: Andrii Nakryiko <andriin@fb.com> > Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com> > --- LGTM, minor nit below. Acked-by: Andrii Nakryiko <andriin@fb.com> > tools/bpf/bpftool/link.c | 44 ++++++++++++++++++++-------------------- > 1 file changed, 22 insertions(+), 22 deletions(-) > > diff --git a/tools/bpf/bpftool/link.c b/tools/bpf/bpftool/link.c > index 670a561dc31b..1ff416eff3d7 100644 > --- a/tools/bpf/bpftool/link.c > +++ b/tools/bpf/bpftool/link.c > @@ -62,6 +62,15 @@ show_link_header_json(struct bpf_link_info *info, json_writer_t *wtr) > jsonw_uint_field(json_wtr, "prog_id", info->prog_id); > } > > +static void show_link_attach_type_json(__u32 attach_type, json_writer_t *wtr) nit: if you look at jsonw_uint_field/jsonw_string_field, they accept json_write_t as a first argument, because they are sort of working on "object" json_writer_t. I think that's good and consistent. No big deal, but if you can adjust it for consistency, it would be good. > +{ > + if (attach_type < ARRAY_SIZE(attach_type_name)) > + jsonw_string_field(wtr, "attach_type", > + attach_type_name[attach_type]); > + else > + jsonw_uint_field(wtr, "attach_type", attach_type); > +} > + [...]
On Tue, Jun 02, 2020 at 12:35 AM CEST, Andrii Nakryiko wrote: > On Sun, May 31, 2020 at 1:32 AM Jakub Sitnicki <jakub@cloudflare.com> wrote: >> >> Code for printing link attach_type is duplicated in a couple of places, and >> likely will be duplicated for future link types as well. Create helpers to >> prevent duplication. >> >> Suggested-by: Andrii Nakryiko <andriin@fb.com> >> Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com> >> --- > > LGTM, minor nit below. > > Acked-by: Andrii Nakryiko <andriin@fb.com> > >> tools/bpf/bpftool/link.c | 44 ++++++++++++++++++++-------------------- >> 1 file changed, 22 insertions(+), 22 deletions(-) >> >> diff --git a/tools/bpf/bpftool/link.c b/tools/bpf/bpftool/link.c >> index 670a561dc31b..1ff416eff3d7 100644 >> --- a/tools/bpf/bpftool/link.c >> +++ b/tools/bpf/bpftool/link.c >> @@ -62,6 +62,15 @@ show_link_header_json(struct bpf_link_info *info, json_writer_t *wtr) >> jsonw_uint_field(json_wtr, "prog_id", info->prog_id); >> } >> >> +static void show_link_attach_type_json(__u32 attach_type, json_writer_t *wtr) > > nit: if you look at jsonw_uint_field/jsonw_string_field, they accept > json_write_t as a first argument, because they are sort of working on > "object" json_writer_t. I think that's good and consistent. No big > deal, but if you can adjust it for consistency, it would be good. I followed show_link_header_json example here. I'm guessing the intention was to keep show_link_header_json and show_link_header_plain consistent, as the former takes an extra arg (wtr). > >> +{ >> + if (attach_type < ARRAY_SIZE(attach_type_name)) >> + jsonw_string_field(wtr, "attach_type", >> + attach_type_name[attach_type]); >> + else >> + jsonw_uint_field(wtr, "attach_type", attach_type); >> +} >> + > > [...]
On Tue, Jun 2, 2020 at 2:37 AM Jakub Sitnicki <jakub@cloudflare.com> wrote: > > On Tue, Jun 02, 2020 at 12:35 AM CEST, Andrii Nakryiko wrote: > > On Sun, May 31, 2020 at 1:32 AM Jakub Sitnicki <jakub@cloudflare.com> wrote: > >> > >> Code for printing link attach_type is duplicated in a couple of places, and > >> likely will be duplicated for future link types as well. Create helpers to > >> prevent duplication. > >> > >> Suggested-by: Andrii Nakryiko <andriin@fb.com> > >> Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com> > >> --- > > > > LGTM, minor nit below. > > > > Acked-by: Andrii Nakryiko <andriin@fb.com> > > > >> tools/bpf/bpftool/link.c | 44 ++++++++++++++++++++-------------------- > >> 1 file changed, 22 insertions(+), 22 deletions(-) > >> > >> diff --git a/tools/bpf/bpftool/link.c b/tools/bpf/bpftool/link.c > >> index 670a561dc31b..1ff416eff3d7 100644 > >> --- a/tools/bpf/bpftool/link.c > >> +++ b/tools/bpf/bpftool/link.c > >> @@ -62,6 +62,15 @@ show_link_header_json(struct bpf_link_info *info, json_writer_t *wtr) > >> jsonw_uint_field(json_wtr, "prog_id", info->prog_id); > >> } > >> > >> +static void show_link_attach_type_json(__u32 attach_type, json_writer_t *wtr) > > > > nit: if you look at jsonw_uint_field/jsonw_string_field, they accept > > json_write_t as a first argument, because they are sort of working on > > "object" json_writer_t. I think that's good and consistent. No big > > deal, but if you can adjust it for consistency, it would be good. > > I followed show_link_header_json example here. I'm guessing the > intention was to keep show_link_header_json and show_link_header_plain > consistent, as the former takes an extra arg (wtr). It's fine, it's a minor point, even though this order feels backwards to me :) > > > > >> +{ > >> + if (attach_type < ARRAY_SIZE(attach_type_name)) > >> + jsonw_string_field(wtr, "attach_type", > >> + attach_type_name[attach_type]); > >> + else > >> + jsonw_uint_field(wtr, "attach_type", attach_type); > >> +} > >> + > > > > [...]
diff --git a/tools/bpf/bpftool/link.c b/tools/bpf/bpftool/link.c index 670a561dc31b..1ff416eff3d7 100644 --- a/tools/bpf/bpftool/link.c +++ b/tools/bpf/bpftool/link.c @@ -62,6 +62,15 @@ show_link_header_json(struct bpf_link_info *info, json_writer_t *wtr) jsonw_uint_field(json_wtr, "prog_id", info->prog_id); } +static void show_link_attach_type_json(__u32 attach_type, json_writer_t *wtr) +{ + if (attach_type < ARRAY_SIZE(attach_type_name)) + jsonw_string_field(wtr, "attach_type", + attach_type_name[attach_type]); + else + jsonw_uint_field(wtr, "attach_type", attach_type); +} + static int get_prog_info(int prog_id, struct bpf_prog_info *info) { __u32 len = sizeof(*info); @@ -105,22 +114,13 @@ static int show_link_close_json(int fd, struct bpf_link_info *info) jsonw_uint_field(json_wtr, "prog_type", prog_info.type); - if (info->tracing.attach_type < ARRAY_SIZE(attach_type_name)) - jsonw_string_field(json_wtr, "attach_type", - attach_type_name[info->tracing.attach_type]); - else - jsonw_uint_field(json_wtr, "attach_type", - info->tracing.attach_type); + show_link_attach_type_json(info->tracing.attach_type, + json_wtr); break; case BPF_LINK_TYPE_CGROUP: jsonw_lluint_field(json_wtr, "cgroup_id", info->cgroup.cgroup_id); - if (info->cgroup.attach_type < ARRAY_SIZE(attach_type_name)) - jsonw_string_field(json_wtr, "attach_type", - attach_type_name[info->cgroup.attach_type]); - else - jsonw_uint_field(json_wtr, "attach_type", - info->cgroup.attach_type); + show_link_attach_type_json(info->cgroup.attach_type, json_wtr); break; default: break; @@ -153,6 +153,14 @@ static void show_link_header_plain(struct bpf_link_info *info) printf("prog %u ", info->prog_id); } +static void show_link_attach_type_plain(__u32 attach_type) +{ + if (attach_type < ARRAY_SIZE(attach_type_name)) + printf("attach_type %s ", attach_type_name[attach_type]); + else + printf("attach_type %u ", attach_type); +} + static int show_link_close_plain(int fd, struct bpf_link_info *info) { struct bpf_prog_info prog_info; @@ -176,19 +184,11 @@ static int show_link_close_plain(int fd, struct bpf_link_info *info) else printf("\n\tprog_type %u ", prog_info.type); - if (info->tracing.attach_type < ARRAY_SIZE(attach_type_name)) - printf("attach_type %s ", - attach_type_name[info->tracing.attach_type]); - else - printf("attach_type %u ", info->tracing.attach_type); + show_link_attach_type_plain(info->tracing.attach_type); break; case BPF_LINK_TYPE_CGROUP: printf("\n\tcgroup_id %zu ", (size_t)info->cgroup.cgroup_id); - if (info->cgroup.attach_type < ARRAY_SIZE(attach_type_name)) - printf("attach_type %s ", - attach_type_name[info->cgroup.attach_type]); - else - printf("attach_type %u ", info->cgroup.attach_type); + show_link_attach_type_plain(info->cgroup.attach_type); break; default: break;
Code for printing link attach_type is duplicated in a couple of places, and likely will be duplicated for future link types as well. Create helpers to prevent duplication. Suggested-by: Andrii Nakryiko <andriin@fb.com> Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com> --- tools/bpf/bpftool/link.c | 44 ++++++++++++++++++++-------------------- 1 file changed, 22 insertions(+), 22 deletions(-)