Re: [PATCH v5 bpf-next 5/6] bpf: add bpf_seq_btf_write helper

From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
To: Alan Maguire <alan.maguire@oracle.com>
Cc: ast@kernel.org, daniel@iogearbox.net, andriin@fb.com, yhs@fb.com,
	linux@rasmusvillemoes.dk, andriy.shevchenko@linux.intel.com,
	pmladek@suse.com, kafai@fb.com, songliubraving@fb.com,
	john.fastabend@gmail.com, kpsingh@chromium.org, shuah@kernel.org,
	rdna@fb.com, scott.branden@broadcom.com, quentin@isovalent.com,
	cneirabustos@gmail.com, jakub@cloudflare.com, mingo@redhat.com,
	rostedt@goodmis.org, bpf@vger.kernel.org, netdev@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-kselftest@vger.kernel.org,
	acme@kernel.org
Subject: Re: [PATCH v5 bpf-next 5/6] bpf: add bpf_seq_btf_write helper
Date: Mon, 21 Sep 2020 18:10:40 -0700	[thread overview]
Message-ID: <20200922011040.osoccsppdljfam4g@ast-mbp.dhcp.thefacebook.com> (raw)
In-Reply-To: <1600436075-2961-6-git-send-email-alan.maguire@oracle.com>

On Fri, Sep 18, 2020 at 02:34:34PM +0100, Alan Maguire wrote:
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 9b89b67..c0815f1 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -3614,6 +3614,15 @@ struct bpf_stack_build_id {
>   *		The number of bytes that were written (or would have been
>   *		written if output had to be truncated due to string size),
>   *		or a negative error in cases of failure.
> + *
> + * long bpf_seq_btf_write(struct seq_file *m, struct btf_ptr *ptr, u32 ptr_size, u64 flags)
> + *	Description
> + *		Use BTF to write to seq_write a string representation of
> + *		*ptr*->ptr, using *ptr*->type name or *ptr*->type_id as per
> + *		bpf_btf_snprintf() above.  *flags* are identical to those
> + *		used for bpf_btf_snprintf.
> + *	Return
> + *		0 on success or a negative error in case of failure.
>   */
>  #define __BPF_FUNC_MAPPER(FN)		\
>  	FN(unspec),			\
> @@ -3766,6 +3775,7 @@ struct bpf_stack_build_id {
>  	FN(d_path),			\
>  	FN(copy_from_user),		\
>  	FN(btf_snprintf),		\
> +	FN(seq_btf_write),		\

bpf_btf_ prefix is a bit mouthful.

May be bpf_print_btf() and bpf_seq_print_btf() ?

> -void btf_type_seq_show(const struct btf *btf, u32 type_id, void *obj,
> -			struct seq_file *m)
> +int btf_type_seq_show_flags(const struct btf *btf, u32 type_id, void *obj,
> +			struct seq_file *m, u64 flags)
>  {
>  	struct btf_show sseq;
>  
>  	sseq.target = m;
>  	sseq.showfn = btf_seq_show;
> -	sseq.flags = BTF_SHOW_NONAME | BTF_SHOW_COMPACT | BTF_SHOW_ZERO |
> -		     BTF_SHOW_UNSAFE;
> +	sseq.flags = flags;

could you roll this change into patch 2?

>  
> -BPF_CALL_5(bpf_btf_snprintf, char *, str, u32, str_size, struct btf_ptr *, ptr,
> -	   u32, btf_ptr_size, u64, flags)
> +static int bpf_btf_printf_prepare(struct btf_ptr *ptr, u32 btf_ptr_size,
> +				  u64 flags, const struct btf **btf,
> +				  s32 *btf_id)

Similarly pls introduce bpf_print_prepare() helper in the patch 3 right away.
It reads odd when something that was just introduced in the previous patch
gets refactored in the next.

The rest looks great to me.
Please address build bot issues and respin.