On Mon, 6 Nov 2017 15:06:31 +0900, Prashant Bhole wrote:
> Added support to show filenames of pinned objects.
> ...
> Signed-off-by: Prashant Bhole <bhole_prashant...@lab.ntt.co.jp>
Thanks for the changes, a couple more nit picks, sorry for not spotting
them earlier.
> v2:
> - Dynamically identify bpf-fs moutpoint
> - Close files descriptors before returning on error
> - Fixed line break for proper output formatting
> - Code style: wrapped lines > 80, used reverse christmastree style
>
> v3:
> - Handle multiple bpffs mountpoints
> - Code style: fixed line break indentation
>
> tools/bpf/bpftool/common.c | 85
> ++++++++++++++++++++++++++++++++++++++++++++++
> tools/bpf/bpftool/main.c | 8 +++++
> tools/bpf/bpftool/main.h | 17 ++++++++++
> tools/bpf/bpftool/map.c | 21 ++++++++++++
> tools/bpf/bpftool/prog.c | 24 +++++++++++++
> 5 files changed, 155 insertions(+)
>
> diff --git a/tools/bpf/bpftool/common.c b/tools/bpf/bpftool/common.c
> index 4556947709ee..152c5bdbe2e9 100644
> --- a/tools/bpf/bpftool/common.c
> +++ b/tools/bpf/bpftool/common.c
> @@ -45,6 +45,8 @@
> #include <sys/mount.h>
> #include <sys/types.h>
> #include <sys/vfs.h>
> +#include <mntent.h>
> +#include <fts.h>
Please try to keep includes in an alphabetical order.
> #include <bpf.h>
>
> @@ -290,3 +292,86 @@ void print_hex_data_json(uint8_t *data, size_t len)
> jsonw_printf(json_wtr, "\"0x%02hhx\"", data[i]);
> jsonw_end_array(json_wtr);
> }
> +
> +int build_pinned_obj_table(struct pinned_obj_table *tab,
> + enum bpf_obj_type type)
> +{
> + struct bpf_prog_info pinned_info = {};
> + __u32 len = sizeof(pinned_info);
> + struct pinned_obj *obj_node = NULL;
> + enum bpf_obj_type objtype;
> + struct mntent *mntent = NULL;
Please try to order variable declarations longest to shortest.
> + FILE *mntfile = NULL;
> + FTSENT *ftse = NULL;
> + FTS *fts = NULL;
> + int fd, err;
> +
> + mntfile = setmntent("/proc/mounts", "r");
> + if (!mntfile)
> + return -1;
> +
> + while ((mntent = getmntent(mntfile)) != NULL) {
Please try to avoid comparisons to NULL, writing:
if (ptr)
is more intuitive to most C programmers than:
if (ptr != NULL)
> + char *path[] = {mntent->mnt_dir, 0};
Shouldn't there be spaces after and before the curly braces? Does
checkpatch --strict not warn about this?
> +
> + if (strncmp(mntent->mnt_type, "bpf", 3) != 0)
> + continue;
> +
> + fts = fts_open(path, 0, NULL);
> + if (!fts)
> + continue;
> +
> + while ((ftse = fts_read(fts)) != NULL) {
> + if (!(ftse->fts_info & FTS_F))
> + continue;
> + fd = open_obj_pinned(ftse->fts_path);
> + if (fd < 0)
> + continue;
> +
> + objtype = get_fd_type(fd);
> + if (objtype != type) {
> + close(fd);
> + continue;
> + }
> + memset(&pinned_info, 0, sizeof(pinned_info));
> + err = bpf_obj_get_info_by_fd(fd, &pinned_info, &len);
> + if (err) {
> + close(fd);
> + continue;
> + }
> +
> + obj_node = malloc(sizeof(*obj_node));
> + if (!obj_node) {
> + close(fd);
> + fts_close(fts);
> + fclose(mntfile);
> + return -1;
> + }
> +
> + memset(obj_node, 0, sizeof(*obj_node));
> + obj_node->id = pinned_info.id;
> + obj_node->path = strdup(ftse->fts_path);
> + hash_add(tab->table, &obj_node->hash, obj_node->id);
> +
> + close(fd);
> + }
> + fts_close(fts);
> + }
> + fclose(mntfile);
> + return 0;
> +}
> +
> +void delete_pinned_obj_table(struct pinned_obj_table *tab)
> +{
> + struct pinned_obj *obj;
> + struct hlist_node *tmp;
> + unsigned int bkt;
> +
> + if (hash_empty(tab->table))
> + return;
is this necessary? Does hash_for_each_safe() not work with empty table?
> + hash_for_each_safe(tab->table, bkt, tmp, obj, hash) {
> + hash_del(&obj->hash);
> + free(obj->path);
> + free(obj);
> + }
> +}
