Re: [PATCH bpf-next 09/13] tools/bpftool: add bpftool support for bpf map element iterator

[Quentin Monnet <quentin@isovalent.com>]

2020-07-13 09:17 UTC-0700 ~ Yonghong Song <yhs@fb.com>
> The optional parameter "map MAP" can be added to "bpftool iter"
> command to create a bpf iterator for map elements. For example,
>   bpftool iter pin ./prog.o /sys/fs/bpf/p1 map id 333
> 
> For map element bpf iterator "map MAP" parameter is required.
> Otherwise, bpf link creation will return an error.
> 
> Signed-off-by: Yonghong Song <yhs@fb.com>
> ---
>  .../bpftool/Documentation/bpftool-iter.rst    | 16 ++++++++--
>  tools/bpf/bpftool/iter.c                      | 32 ++++++++++++++++---
>  2 files changed, 42 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/bpf/bpftool/Documentation/bpftool-iter.rst b/tools/bpf/bpftool/Documentation/bpftool-iter.rst
> index 8dce698eab79..53ee4fb188b4 100644
> --- a/tools/bpf/bpftool/Documentation/bpftool-iter.rst
> +++ b/tools/bpf/bpftool/Documentation/bpftool-iter.rst
> @@ -17,14 +17,15 @@ SYNOPSIS
>  ITER COMMANDS
>  ===================
>  
> -|	**bpftool** **iter pin** *OBJ* *PATH*
> +|	**bpftool** **iter pin** *OBJ* *PATH* [**map** *MAP*]
>  |	**bpftool** **iter help**
>  |
>  |	*OBJ* := /a/file/of/bpf_iter_target.o
> +|       *MAP* := { **id** *MAP_ID* | **pinned** *FILE* }

Please don't change the indentation style (other lines have a tab).

>  
>  DESCRIPTION
>  ===========
> -	**bpftool iter pin** *OBJ* *PATH*
> +	**bpftool iter pin** *OBJ* *PATH* [**map** *MAP*]
>  		  A bpf iterator combines a kernel iterating of
>  		  particular kernel data (e.g., tasks, bpf_maps, etc.)
>  		  and a bpf program called for each kernel data object
> @@ -37,6 +38,10 @@ DESCRIPTION
>  		  character ('.'), which is reserved for future extensions
>  		  of *bpffs*.
>  
> +                  Map element bpf iterator requires an additional parameter
> +                  *MAP* so bpf program can iterate over map elements for
> +                  that map.
> +

Same note on indentation.

Could you please also explain in a few words what the "Map element bpf
iterator" is? Reusing part of your cover letter (see below) could do,
it's just so that users not familiar with the concept can get an idea of
what it does.

---
User can have a bpf program in kernel to run with each map element,
do checking, filtering, aggregation, etc. without copying data
to user space.
---

>  		  User can then *cat PATH* to see the bpf iterator output.
>  
>  	**bpftool iter help**

[...]

> @@ -62,13 +83,16 @@ static int do_pin(int argc, char **argv)
>  	bpf_link__destroy(link);
>  close_obj:
>  	bpf_object__close(obj);
> +close_map_fd:
> +	if (map_fd >= 0)
> +		close(map_fd);
>  	return err;
>  }
>  
>  static int do_help(int argc, char **argv)
>  {
>  	fprintf(stderr,
> -		"Usage: %1$s %2$s pin OBJ PATH\n"
> +		"Usage: %1$s %2$s pin OBJ PATH [map MAP]\n"

You probably want to add HELP_SPEC_MAP (as in map.c) to tell the user
what MAP should be.

Could you please also update the bash completion?

Thanks,
Quentin