[DTrace-devel] [PATCH 04/16] Replace probe_fini with detach, and separate detach and destroy

[Eugene Loh]

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

Two other comments below:

On 3/19/21 12:54 AM, Kris Van Hees wrote:
> The probe framework provides a function dt_probe_fini() that destroys
> all probes.  This function calls dt_probe_destroy() on every probe,
> and it was calling the probe_fini hook for each probe to perform the
> detach and destroy.  Some providers will need additional work done
> between the detach and the destroy, so those need to be implemented
> as separate hooks.  It is also desirable to explicitly perform the
> per-probe detach as a termination of the earlier attach.

Starting the commit message with dt_probe_fini() is a little funny since 
it doesn't really have much to do with this patch.  Further, while I get 
the gist of what you're doing from the commit message, a few more 
concrete steps would have helped me a lot with the nuts and bolts.  How 
about the following commit message instead?

The dt_probe_destroy() function calls the probe_fini() hook, which
both detaches and destroys probes.  Some providers will need additional
work done between the detach and the destroy, so they need to be
implemented as separate hooks.  Therefore:

*)  Remove probe_destroy() from probe_fini(), and
     remove tp_probe_destroy() from tp_probe_fini().

*)  In dt_probe_destroy(), replace the call to probe_fini()
     with a call to probe_destroy().

*)  Rename probe_fini() to detach(), and
     rename tp_probe_fini() to tp_detach().

*)  Introduce a new dt_probe_detach() (that loops over probes,
     calling the newly named detach hooks) and call it early
     in dtrace_close().

Also...
> diff --git a/libdtrace/dt_probe.c b/libdtrace/dt_probe.c
> +void
> +dt_probe_detach(dtrace_hdl_t *dtp)
> +{
> +	uint32_t	i;
> +
> +	for (i = 0; i < dtp->dt_probes_sz; i++) {
> +		dt_probe_t	*prp = dtp->dt_probes[i];
> +
> +		if (prp == NULL)
> +			continue;
> +
> +		if (prp->prov && prp->prov->impl && prp->prov->impl->detach)
> +			prp->prov->impl->detach(dtp, prp);
> +	}
> +}

Another way of writing the loop body is:
         if (prp &&
             prp->prov &&
             prp->prov->impl &&
             prp->prov->impl->detach)
                 prp->prov->impl->detach(dtp, prp);
In my opinion, this is easier to read.