[DTrace-devel] [PATCH 1/2] Clean up error reporting in the BPF progran load process

[Eugene Loh]

Reviewed-by: Eugene Loh <eugene.loh at oracle.com>

Is the "correct" behavior documented anywhere?  If not, how are we to 
judge who is misusing what?

Does any test show any problem?

It appears that when attach() encounters an error, it sometimes returns:
--  -ENOENT, or
--  -1, or
--  dt_set_errno(dtp, errno)
To be more cohesive, should it (well, its multiple implementations) just 
return -1?  Its behavior might as well be coherent.

I know this is outside the scope of this patch, but dt_tp_attach() has:
         if (tpp->event_id == -1)
                 return 0;
I'm rusty on this stuff, but why is event_id==-1 okay?

And...

On 9/12/22 13:04, Kris Van Hees via DTrace-devel wrote:
> The return value for the attach() provider hook was being passed to
> the dtrace_go() through dt_bpf_load_progs(), confusing the dtrace
> command line utility which was expecting 0 or -1 (with the proper
> error code being set).
>
> It turns out return values were misused in a few other places.  The
> code will now explicitly check for -1 as failure indication (and return
> -1 explicitly where needed) for dt_aggregate_go(), dt_bpf_gmap_create(),
> dt_bpf_load_prog(), and dt_bpf_load_progs().

This implies that when dt_bpf_load_progs() checks dt_bpf_load_prog() for 
errors, it should check ==-1 rather than <0.

> diff --git a/libdtrace/dt_bpf.c b/libdtrace/dt_bpf.c
> @@ -1059,15 +1059,13 @@ dt_bpf_load_progs(dtrace_hdl_t *dtp, uint_t cflags)
>   
>   		fd = dt_bpf_load_prog(dtp, prp, dp, cflags);
>   		if (fd < 0)
> -			return fd;
> +			return -1;

Here!  Shouldn't we check fd==-1 rather than fd<0?  This presumably does 
not matter, but it seems to be what the commit message is advertising.

>   		dt_difo_free(dtp, dp);
>   
> -		if (!prp->prov->impl->attach)
> -			return -1;
> -		rc = prp->prov->impl->attach(dtp, prp, fd);
> -		if (rc < 0)
> -			return rc;
> +		if (!prp->prov->impl->attach ||
> +		    prp->prov->impl->attach(dtp, prp, fd) < 0)
> +			return dt_set_errno(dtp, EDT_ENABLING_ERR);
>   	}
>   
>   	return 0;
> diff --git a/libdtrace/dt_work.c b/libdtrace/dt_work.c
> @@ -69,17 +68,13 @@ dtrace_go(dtrace_hdl_t *dtp, uint_t cflags)
>                   setrlimit(RLIMIT_MEMLOCK, &rl);
>           }
>   
> -	/*
> -	 * Create the global BPF maps.  This is done only once regardless of
> -	 * how many programs there are.
> -	 */
> -	err = dt_bpf_gmap_create(dtp);
> -	if (err)
> -		return err;
> +	/* Create the global BPF maps. */
> +	if (dt_bpf_gmap_create(dtp) == -1)
> +		return -1;
>   
> -	err = dt_bpf_load_progs(dtp, cflags);
> -	if (err)
> -		return err;
> +	/* Load the BPF programs.  */

Usually (like a few lines earlier), only one space before "*/".

> +	if (dt_bpf_load_progs(dtp, cflags) == -1)
> +		return -1;
>   
>   	/*
>   	 * Set up the event polling file descriptor.