Re: [PATCH bpf-next 1/9] selftests: bpf: move tracing helpers to trace_helper

On Tue, Nov 17, 2020 at 02:56:36PM +0000, Daniel T. Lee wrote:
> Under the samples/bpf directory, similar tracing helpers are
> fragmented around. To keep consistent of tracing programs, this commit
> moves the helper and define locations to increase the reuse of each
> helper function.
> 
> Signed-off-by: Daniel T. Lee <[email protected]>
> 
> ---
>  samples/bpf/Makefile                        |  2 +-
>  samples/bpf/hbm.c                           | 51 ++++-----------------
>  tools/testing/selftests/bpf/trace_helpers.c | 33 ++++++++++++-
>  tools/testing/selftests/bpf/trace_helpers.h |  3 ++
>  4 files changed, 45 insertions(+), 44 deletions(-)
> 
> diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile
> index aeebf5d12f32..3e83cd5ca1c2 100644
> --- a/samples/bpf/Makefile
> +++ b/samples/bpf/Makefile
> @@ -110,7 +110,7 @@ xdp_fwd-objs := xdp_fwd_user.o
>  task_fd_query-objs := bpf_load.o task_fd_query_user.o $(TRACE_HELPERS)
>  xdp_sample_pkts-objs := xdp_sample_pkts_user.o $(TRACE_HELPERS)
>  ibumad-objs := bpf_load.o ibumad_user.o $(TRACE_HELPERS)
> -hbm-objs := bpf_load.o hbm.o $(CGROUP_HELPERS)
> +hbm-objs := bpf_load.o hbm.o $(CGROUP_HELPERS) $(TRACE_HELPERS)
>  
>  # Tell kbuild to always build the programs
>  always-y := $(tprogs-y)
> diff --git a/samples/bpf/hbm.c b/samples/bpf/hbm.c
> index 400e741a56eb..b9f9f771dd81 100644
> --- a/samples/bpf/hbm.c
> +++ b/samples/bpf/hbm.c
> @@ -48,6 +48,7 @@
>  
>  #include "bpf_load.h"
>  #include "bpf_rlimit.h"
> +#include "trace_helpers.h"
>  #include "cgroup_helpers.h"
>  #include "hbm.h"
>  #include "bpf_util.h"
> @@ -65,51 +66,12 @@ bool no_cn_flag;
>  bool edt_flag;
>  
>  static void Usage(void);
> -static void read_trace_pipe2(void);
>  static void do_error(char *msg, bool errno_flag);
>  
> -#define DEBUGFS "/sys/kernel/debug/tracing/"
> -
>  struct bpf_object *obj;
>  int bpfprog_fd;
>  int cgroup_storage_fd;
>  
> -static void read_trace_pipe2(void)
> -{
> -	int trace_fd;
> -	FILE *outf;
> -	char *outFname = "hbm_out.log";
> -
> -	trace_fd = open(DEBUGFS "trace_pipe", O_RDONLY, 0);
> -	if (trace_fd < 0) {
> -		printf("Error opening trace_pipe\n");
> -		return;
> -	}
> -
> -//	Future support of ingress
> -//	if (!outFlag)
> -//		outFname = "hbm_in.log";
> -	outf = fopen(outFname, "w");
> -
> -	if (outf == NULL)
> -		printf("Error creating %s\n", outFname);
> -
> -	while (1) {
> -		static char buf[4097];
> -		ssize_t sz;
> -
> -		sz = read(trace_fd, buf, sizeof(buf) - 1);
> -		if (sz > 0) {
> -			buf[sz] = 0;
> -			puts(buf);
> -			if (outf != NULL) {
> -				fprintf(outf, "%s\n", buf);
> -				fflush(outf);
> -			}
> -		}
> -	}
> -}
> -
>  static void do_error(char *msg, bool errno_flag)
>  {
>  	if (errno_flag)
> @@ -392,8 +354,15 @@ static int run_bpf_prog(char *prog, int cg_id)
>  		fclose(fout);
>  	}
>  
> -	if (debugFlag)
> -		read_trace_pipe2();
> +	if (debugFlag) {
> +		char *out_fname = "hbm_out.log";
> +		/* Future support of ingress */
> +		// if (!outFlag)
> +		//	out_fname = "hbm_in.log";
> +
> +		read_trace_pipe2(out_fname);
> +	}
> +
>  	return rc;
>  err:
>  	rc = 1;
> diff --git a/tools/testing/selftests/bpf/trace_helpers.c b/tools/testing/selftests/bpf/trace_helpers.c
> index 1bbd1d9830c8..b7c184e109e8 100644
> --- a/tools/testing/selftests/bpf/trace_helpers.c
> +++ b/tools/testing/selftests/bpf/trace_helpers.c
> @@ -11,8 +11,6 @@
>  #include <sys/mman.h>
>  #include "trace_helpers.h"
>  
> -#define DEBUGFS "/sys/kernel/debug/tracing/"
Is this change needed?

> -
>  #define MAX_SYMS 300000
>  static struct ksym syms[MAX_SYMS];
>  static int sym_cnt;
> @@ -136,3 +134,34 @@ void read_trace_pipe(void)
>  		}
>  	}
>  }
> +
> +void read_trace_pipe2(char *filename)
> +{
> +	int trace_fd;
> +	FILE *outf;
> +
> +	trace_fd = open(DEBUGFS "trace_pipe", O_RDONLY, 0);
> +	if (trace_fd < 0) {
> +		printf("Error opening trace_pipe\n");
> +		return;
> +	}
> +
> +	outf = fopen(filename, "w");
> +	if (!outf)
> +		printf("Error creating %s\n", filename);
> +
> +	while (1) {
> +		static char buf[4096];
> +		ssize_t sz;
> +
> +		sz = read(trace_fd, buf, sizeof(buf) - 1);
> +		if (sz > 0) {
> +			buf[sz] = 0;
> +			puts(buf);
> +			if (outf) {
> +				fprintf(outf, "%s\n", buf);
> +				fflush(outf);
> +			}
> +		}
> +	}
It needs a fclose().

IIUC, this function will never return.  I am not sure
this is something that should be made available to selftests.