Message ID | 20191220230301.888477-1-hechaol@fb.com |
---|---|
State | Changes Requested |
Delegated to: | BPF Maintainers |
Headers | show |
Series | [bpf-next] bpf: Print error message for bpftool cgroup show | expand |
On Fri, Dec 20, 2019 at 3:04 PM Hechao Li <hechaol@fb.com> wrote: > For next revision, please don't forget to add "PATCH" in patch prefix. So you'll have: [PATCH v2 bpf-next] <subject here> for you v2. > Currently, when bpftool cgroup show <path> has an error, no error > message is printed. This is confusing because the user may think the > result is empty. > > Before the change: > > $ bpftool cgroup show /sys/fs/cgroup > ID AttachType AttachFlags Name > $ echo $? > 255 > > After the change: > $ ./bpftool cgroup show /sys/fs/cgroup > Error: can't query bpf programs attached to /sys/fs/cgroup: Operation > not permitted > > Signed-off-by: Hechao Li <hechaol@fb.com> > --- > tools/bpf/bpftool/cgroup.c | 60 ++++++++++++++++++++++++++------------ > 1 file changed, 42 insertions(+), 18 deletions(-) > > diff --git a/tools/bpf/bpftool/cgroup.c b/tools/bpf/bpftool/cgroup.c > index 1ef45e55039e..74b57e2d7524 100644 > --- a/tools/bpf/bpftool/cgroup.c > +++ b/tools/bpf/bpftool/cgroup.c > @@ -117,6 +117,28 @@ static int count_attached_bpf_progs(int cgroup_fd, enum bpf_attach_type type) > return prog_cnt; > } > > +#define QUERY_CGROUP_ERR (-1) > +#define QUERY_CGROUP_NO_PROG (0) > +#define QUERY_CGROUP_SUCCESS (1) > +static int check_query_cgroup_progs(int cgroup_fd) > +{ How about calling it a bit differently and use typical bool + error return values. E.g., "cgroup_has_attached_progs" (or something along those lines), then 1 means "true, it has", 0 means "false, doesn't have any BPF progs", <0 means error, as usual. No need for special QUERY* constants. > + enum bpf_attach_type type; > + bool no_prog = true; > + > + for (type = 0; type < __MAX_BPF_ATTACH_TYPE; type++) { > + int count = count_attached_bpf_progs(cgroup_fd, type); > + > + if (count < 0 && errno != EINVAL) > + return QUERY_CGROUP_ERR; > + > + if (count > 0) { > + no_prog = false; > + break; > + } > + } > + > + return no_prog ? QUERY_CGROUP_NO_PROG : QUERY_CGROUP_SUCCESS; > +} > static int show_attached_bpf_progs(int cgroup_fd, enum bpf_attach_type type, > int level) > { > @@ -162,6 +184,7 @@ static int do_show(int argc, char **argv) > { > enum bpf_attach_type type; > const char *path; > + int query_check; > int cgroup_fd; > int ret = -1; > > @@ -192,6 +215,16 @@ static int do_show(int argc, char **argv) > goto exit; you just removed exit label below, this causes compilation error here > } > > + query_check = check_query_cgroup_progs(cgroup_fd); > + if (query_check == QUERY_CGROUP_ERR) { > + p_err("can't query bpf programs attached to %s: %s", > + path, strerror(errno)); > + goto exit_cgroup; > + } else if (query_check == QUERY_CGROUP_NO_PROG) { > + ret = 0; > + goto exit_cgroup; > + } > + [...]
diff --git a/tools/bpf/bpftool/cgroup.c b/tools/bpf/bpftool/cgroup.c index 1ef45e55039e..74b57e2d7524 100644 --- a/tools/bpf/bpftool/cgroup.c +++ b/tools/bpf/bpftool/cgroup.c @@ -117,6 +117,28 @@ static int count_attached_bpf_progs(int cgroup_fd, enum bpf_attach_type type) return prog_cnt; } +#define QUERY_CGROUP_ERR (-1) +#define QUERY_CGROUP_NO_PROG (0) +#define QUERY_CGROUP_SUCCESS (1) +static int check_query_cgroup_progs(int cgroup_fd) +{ + enum bpf_attach_type type; + bool no_prog = true; + + for (type = 0; type < __MAX_BPF_ATTACH_TYPE; type++) { + int count = count_attached_bpf_progs(cgroup_fd, type); + + if (count < 0 && errno != EINVAL) + return QUERY_CGROUP_ERR; + + if (count > 0) { + no_prog = false; + break; + } + } + + return no_prog ? QUERY_CGROUP_NO_PROG : QUERY_CGROUP_SUCCESS; +} static int show_attached_bpf_progs(int cgroup_fd, enum bpf_attach_type type, int level) { @@ -162,6 +184,7 @@ static int do_show(int argc, char **argv) { enum bpf_attach_type type; const char *path; + int query_check; int cgroup_fd; int ret = -1; @@ -192,6 +215,16 @@ static int do_show(int argc, char **argv) goto exit; } + query_check = check_query_cgroup_progs(cgroup_fd); + if (query_check == QUERY_CGROUP_ERR) { + p_err("can't query bpf programs attached to %s: %s", + path, strerror(errno)); + goto exit_cgroup; + } else if (query_check == QUERY_CGROUP_NO_PROG) { + ret = 0; + goto exit_cgroup; + } + if (json_output) jsonw_start_array(json_wtr); else @@ -212,8 +245,8 @@ static int do_show(int argc, char **argv) if (json_output) jsonw_end_array(json_wtr); +exit_cgroup: close(cgroup_fd); -exit: return ret; } @@ -228,7 +261,7 @@ static int do_show_tree_fn(const char *fpath, const struct stat *sb, int typeflag, struct FTW *ftw) { enum bpf_attach_type type; - bool skip = true; + int query_check; int cgroup_fd; if (typeflag != FTW_D) @@ -240,22 +273,13 @@ static int do_show_tree_fn(const char *fpath, const struct stat *sb, return SHOW_TREE_FN_ERR; } - for (type = 0; type < __MAX_BPF_ATTACH_TYPE; type++) { - int count = count_attached_bpf_progs(cgroup_fd, type); - - if (count < 0 && errno != EINVAL) { - p_err("can't query bpf programs attached to %s: %s", - fpath, strerror(errno)); - close(cgroup_fd); - return SHOW_TREE_FN_ERR; - } - if (count > 0) { - skip = false; - break; - } - } - - if (skip) { + query_check = check_query_cgroup_progs(cgroup_fd); + if (query_check == QUERY_CGROUP_ERR) { + p_err("can't query bpf programs attached to %s: %s", + fpath, strerror(errno)); + close(cgroup_fd); + return SHOW_TREE_FN_ERR; + } else if (query_check == QUERY_CGROUP_NO_PROG) { close(cgroup_fd); return 0; }
Currently, when bpftool cgroup show <path> has an error, no error message is printed. This is confusing because the user may think the result is empty. Before the change: $ bpftool cgroup show /sys/fs/cgroup ID AttachType AttachFlags Name $ echo $? 255 After the change: $ ./bpftool cgroup show /sys/fs/cgroup Error: can't query bpf programs attached to /sys/fs/cgroup: Operation not permitted Signed-off-by: Hechao Li <hechaol@fb.com> --- tools/bpf/bpftool/cgroup.c | 60 ++++++++++++++++++++++++++------------ 1 file changed, 42 insertions(+), 18 deletions(-)