From mboxrd@z Thu Jan  1 00:00:00 1970
From: Jakub Kicinski <jakub.kicinski@netronome.com>
Subject: Re: [PATCH net-next V3 2/3] tools: bpftool: show filenames of
 pinned objects
Date: Tue, 7 Nov 2017 04:36:56 -0800
Message-ID: <20171107043656.769e4124@cakuba>
References: <20171106060632.3380-1-bhole_prashant_q7@lab.ntt.co.jp>
        <20171106060632.3380-3-bhole_prashant_q7@lab.ntt.co.jp>
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Cc: "David S . Miller" <davem@davemloft.net>, netdev@vger.kernel.org,
        Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Quentin Monnet <quentin.monnet@netronome.com>
To: Prashant Bhole <bhole_prashant_q7@lab.ntt.co.jp>
Return-path: <netdev-owner@vger.kernel.org>
Received: from mail-pg0-f66.google.com ([74.125.83.66]:50919 "EHLO
        mail-pg0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1750720AbdKGMhH (ORCPT
        <rfc822;netdev@vger.kernel.org>); Tue, 7 Nov 2017 07:37:07 -0500
Received: by mail-pg0-f66.google.com with SMTP id y5so10921791pgq.7
        for <netdev@vger.kernel.org>; Tue, 07 Nov 2017 04:37:07 -0800 (PST)
In-Reply-To: <20171106060632.3380-3-bhole_prashant_q7@lab.ntt.co.jp>
Sender: netdev-owner@vger.kernel.org
List-ID: <netdev.vger.kernel.org>

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_q7@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);
> +	}
> +}