Re: [PATCH 1/2] Fix probe discovery loop

[Kris Van Hees <kris.van.hees@oracle.com>]

While this is essentially a good fix, I am going to drop it in favour of
just looping over dt_providers[] for this.  Since the discover implementation
already expects that upon being called, if new providers need to be created,
it also creates the probes associated with them, there is no need to call
it in every provider anyway.

In the future, we might re-engineer this to work differently, but since that
is not the case yet, the easier solution seems more prudent.

On Sun, Mar 01, 2026 at 05:39:42PM -0500, eugene.loh@oracle.com wrote:
> From: Eugene Loh <eugene.loh@oracle.com>
> 
> In dt_provider_discover(), we loop over providers to discover new
> probes.  For each existing provider whose implementation has a discovery
> function, we call the function, thereby possibly adding new providers.
> 
> This has at least two problems:
> 
>   x The iterator is not designed to handle new table elements
>     being added during the loop.
> 
>   x New providers that are added will presumably use the same
>     implementation that discovered them.  So, new providers are
>     simply adding redundant discovery functions.
> 
> While neither of these problems apparently causes any errors, the result
> is a messy loop that leads potentially to unnecessary probe discovery.
> 
> Add dtp->dt_discs, a linked list of provider discovery functions.
> 
> Add functions to init and free this linked list.
> 
> Replace the iteration over a growing list of providers with an iteration
> over the small, fixed list of provider discovery functions.
> 
> Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
> ---
>  libdtrace/dt_impl.h     |  1 +
>  libdtrace/dt_open.c     |  6 ++++++
>  libdtrace/dt_provider.c | 45 +++++++++++++++++++++++++++++++++++------
>  libdtrace/dt_provider.h |  2 ++
>  4 files changed, 48 insertions(+), 6 deletions(-)
> 
> diff --git a/libdtrace/dt_impl.h b/libdtrace/dt_impl.h
> index 5282efbdb..ff730319b 100644
> --- a/libdtrace/dt_impl.h
> +++ b/libdtrace/dt_impl.h
> @@ -333,6 +333,7 @@ struct dtrace_hdl {
>  	struct dt_probe *dt_error; /* ERROR probe */
>  
>  	dt_htab_t *dt_provs;	/* hash table of dt_provider_t's */
> +	dt_list_t dt_discs;	/* linked list of provider discovery functions */
>  	const struct dt_provider *dt_prov_pid; /* PID provider */
>  	int dt_proc_signal;	/* signal used to interrupt monitoring threads */
>  	struct sigaction dt_proc_oact;
> diff --git a/libdtrace/dt_open.c b/libdtrace/dt_open.c
> index a45908047..224f60cf7 100644
> --- a/libdtrace/dt_open.c
> +++ b/libdtrace/dt_open.c
> @@ -1250,6 +1250,11 @@ dtrace_init(dtrace_hdl_t *dtp)
>  			   dt_providers[i]->name);
>  	}
>  
> +	/*
> +	 * Initialize the list of provider discovery functions.
> +	 */
> +	dt_provider_discs_init(dtp);
> +
>  	return 0;
>  }
>  
> @@ -1342,6 +1347,7 @@ dtrace_close(dtrace_hdl_t *dtp)
>  	dt_probe_fini(dtp);
>  
>  	dt_htab_destroy(dtp->dt_provs);
> +	dt_provider_discs_free(dtp);
>  
>  	for (i = 1; i < dtp->dt_cpp_argc; i++)
>  		free(dtp->dt_cpp_argv[i]);
> diff --git a/libdtrace/dt_provider.c b/libdtrace/dt_provider.c
> index 079265b72..f7f76f9c2 100644
> --- a/libdtrace/dt_provider.c
> +++ b/libdtrace/dt_provider.c
> @@ -231,18 +231,51 @@ dt_provider_xref(dtrace_hdl_t *dtp, dt_provider_t *pvp, id_t id)
>  	return 0;
>  }
>  
> -int
> -dt_provider_discover(dtrace_hdl_t *dtp)
> +typedef struct list_disc {
> +	dt_list_t	list;
> +	int		(*disc)(dtrace_hdl_t *dtp);
> +} list_disc_t;
> +
> +void
> +dt_provider_discs_init(dtrace_hdl_t *dtp)
>  {
> -	int		prid = dtp->dt_probe_id;
>  	dt_htab_next_t	*it = NULL;
>  	dt_provider_t	*pvp;
>  
> -	/* Discover new probes. */
>  	while ((pvp = dt_htab_next(dtp->dt_provs, &it)) != NULL) {
> -		if (pvp->impl->discover && pvp->impl->discover(dtp) < 0)
> -			return -1;        /* errno is already set */
> +		list_disc_t *disc;
> +
> +		if (pvp->impl->discover == NULL)
> +			continue;
> +		disc = dt_alloc(dtp, sizeof(list_disc_t));
> +		if (disc == NULL)
> +			return;
> +		disc->disc = pvp->impl->discover;
> +		dt_list_append(&dtp->dt_discs, disc);
>  	}
> +}
> +
> +void
> +dt_provider_discs_free(dtrace_hdl_t *dtp)
> +{
> +	list_disc_t *disc;
> +
> +	while ((disc = dt_list_next(&dtp->dt_discs)) != NULL) {
> +		dt_list_delete(&dtp->dt_discs, disc);
> +		dt_free(dtp, disc);
> +	}
> +}
> +
> +int
> +dt_provider_discover(dtrace_hdl_t *dtp)
> +{
> +	int		prid = dtp->dt_probe_id;
> +	list_disc_t	*disc;
> +
> +	/* Discover new probes. */
> +	for (disc = dt_list_next(&dtp->dt_discs); disc; disc = dt_list_next(disc))
> +		if (disc->disc(dtp) < 0)
> +			return -1;
>  
>  	/* Add them. */
>  	for ( ; prid < dtp->dt_probe_id; prid++) {
> diff --git a/libdtrace/dt_provider.h b/libdtrace/dt_provider.h
> index dc1266711..0f8c13a00 100644
> --- a/libdtrace/dt_provider.h
> +++ b/libdtrace/dt_provider.h
> @@ -120,6 +120,8 @@ extern dt_provider_t *dt_provider_create(dtrace_hdl_t *, const char *,
>  					 const dtrace_pattr_t *, void *);
>  extern int dt_provider_xref(dtrace_hdl_t *, dt_provider_t *, id_t);
>  extern int dt_provider_discover(dtrace_hdl_t *dtp);
> +extern void dt_provider_discs_init(dtrace_hdl_t *dtp);
> +extern void dt_provider_discs_free(dtrace_hdl_t *dtp);
>  
>  #ifdef	__cplusplus
>  }
> -- 
> 2.47.3
>