Re: [PATCH v4 02/19] Add a dt_provider_discover() function

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

On Mon, Oct 28, 2024 at 01:18:35AM -0400, eugene.loh@oracle.com wrote:
> From: Eugene Loh <eugene.loh@oracle.com>
> 
> It is possible for new providers and probes to appear after dtrace_go().
> Add a function that can be called for such discovery, on a per-provider
> basis.  Each provider that supports such discovery needs to provide:
> 
> *)  a function discover() that performs discovery on behalf of the provider
> 
> *)  a function add_probe() that adds a particular (discovered) probe
> 
> To support looping over providers, move dt_providers[] from
> dt_open.c to dt_provider.c and extern it in dt_provider.h.
> 
> Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
> ---
>  libdtrace/dt_bpf.c      |  4 +++
>  libdtrace/dt_open.c     | 22 +---------------
>  libdtrace/dt_provider.c | 57 +++++++++++++++++++++++++++++++++++++++++
>  libdtrace/dt_provider.h |  6 +++++
>  libdtrace/dt_work.c     |  3 +++
>  5 files changed, 71 insertions(+), 21 deletions(-)
> 
> diff --git a/libdtrace/dt_bpf.c b/libdtrace/dt_bpf.c
> index ad11d178e..856110306 100644
> --- a/libdtrace/dt_bpf.c
> +++ b/libdtrace/dt_bpf.c
> @@ -1275,6 +1275,10 @@ dt_bpf_load_progs(dtrace_hdl_t *dtp, uint_t cflags)
>  		int		fd;
>  		int		rc = -1;
>  
> +		/* Handle probes discovered after compilation. */
> +		if (prp->prov->impl->add_probe)
> +			prp->prov->impl->add_probe(dtp, prp);
> +
>  		dp = prp->difo;
>  		if (dp == NULL)
>  			continue;
> diff --git a/libdtrace/dt_open.c b/libdtrace/dt_open.c
> index 848141ddc..1f586fc4f 100644
> --- a/libdtrace/dt_open.c
> +++ b/libdtrace/dt_open.c
> @@ -61,26 +61,6 @@ const dt_version_t _dtrace_versions[] = {
>  	0
>  };
>  
> -/*
> - * List of provider modules that register providers and probes.  A single
> - * provider module may create multiple providers.
> - */
> -static const dt_provimpl_t *dt_providers[] = {
> -	&dt_dtrace,		/* list dt_dtrace first */
> -	&dt_cpc,
> -	&dt_fbt_fprobe,
> -	&dt_io,
> -	&dt_ip,
> -	&dt_lockstat,
> -	&dt_proc,
> -	&dt_profile,
> -	&dt_rawtp,
> -	&dt_sched,
> -	&dt_sdt,
> -	&dt_syscall,
> -	&dt_uprobe,
> -};
> -
>  /*
>   * Table of global identifiers.  This is used to populate the global identifier
>   * hash when a new dtrace client open occurs.  For more info see dt_ident.h.
> @@ -1210,7 +1190,7 @@ dtrace_init(dtrace_hdl_t *dtp)
>  	 * known providers.
>  	 */
>  	dt_probe_init(dtp);
> -	for (i = 0; i < ARRAY_SIZE(dt_providers); i++) {
> +	for (i = 0; dt_providers[i]; i++) {
>  		int	n;
>  
>  		n = dt_providers[i]->populate(dtp);
> diff --git a/libdtrace/dt_provider.c b/libdtrace/dt_provider.c
> index 8cfef0ba2..5f9766183 100644
> --- a/libdtrace/dt_provider.c
> +++ b/libdtrace/dt_provider.c
> @@ -18,10 +18,32 @@
>  #include <port.h>
>  
>  #include <dt_provider.h>
> +#include <dt_probe.h>
>  #include <dt_module.h>
>  #include <dt_string.h>
>  #include <dt_list.h>
>  
> +/*
> + * List of provider modules that register providers and probes.  A single
> + * provider module may create multiple providers.
> + */
> +const dt_provimpl_t *dt_providers[] = {
> +	&dt_dtrace,		/* list dt_dtrace first */
> +	&dt_cpc,
> +	&dt_fbt_fprobe,
> +	&dt_io,
> +	&dt_ip,
> +	&dt_lockstat,
> +	&dt_proc,
> +	&dt_profile,
> +	&dt_rawtp,
> +	&dt_sched,
> +	&dt_sdt,
> +	&dt_syscall,
> +	&dt_uprobe,
> +	NULL
> +};
> +
>  static uint32_t
>  dt_provider_hval(const dt_provider_t *pvp)
>  {
> @@ -149,3 +171,38 @@ dt_provider_xref(dtrace_hdl_t *dtp, dt_provider_t *pvp, id_t id)
>  	BT_SET(pvp->pv_xrefs, id);
>  	return 0;
>  }
> +
> +int
> +dt_provider_discover(dtrace_hdl_t *dtp)
> +{
> +	int i, prid = dtp->dt_probe_id;
> +
> +	/* Discover new probes. */
> +	for (i = 0; dt_providers[i]; i++) {
> +		int n;
> +
> +		if (dt_providers[i]->discover == NULL)
> +			continue;
> +
> +		n = dt_providers[i]->discover(dtp);
> +		if (n < 0)
> +			return -1;        /* errno is already set */
> +	}

My apologies for not seeing this sooner, but can't this simply be:

	for (i = 0; dt_providers[i]; i++) {
		if (dt_providers[i]->discover && dt_providers[i]->discover(dtp) < 0)
			return -1;        /* errno is already set */
	}


> +
> +	/* Add them. */
> +	for ( ; prid < dtp->dt_probe_id; prid++) {
> +		dt_probe_t	*prp = dtp->dt_probes[prid];
> +		int		rc;
> +
> +		dt_probe_enable(dtp, prp);
> +
> +		if (prp->prov->impl->add_probe == NULL)
> +			continue;
> +
> +		rc = prp->prov->impl->add_probe(dtp, prp);
> +		if (rc < 0)
> +			return rc;
> +	}
> +
> +	return 0;
> +}
> diff --git a/libdtrace/dt_provider.h b/libdtrace/dt_provider.h
> index 17b1844cb..384d4fd33 100644
> --- a/libdtrace/dt_provider.h
> +++ b/libdtrace/dt_provider.h
> @@ -62,6 +62,9 @@ typedef struct dt_provimpl {
>  	int (*probe_info)(dtrace_hdl_t *dtp,	/* get probe info */
>  			  const struct dt_probe *prp,
>  			  int *argcp, dt_argdesc_t **argvp);
> +	int (*discover)(dtrace_hdl_t *dtp);	/* discover new probes */
> +	int (*add_probe)(dtrace_hdl_t *dtp,	/* add a new probe */
> +			 struct dt_probe *prp);
>  	void (*detach)(dtrace_hdl_t *dtp,	/* probe cleanup */
>  		       const struct dt_probe *prb);
>  	void (*probe_destroy)(dtrace_hdl_t *dtp, /* free probe data */
> @@ -86,6 +89,8 @@ extern dt_provimpl_t dt_sdt;
>  extern dt_provimpl_t dt_syscall;
>  extern dt_provimpl_t dt_uprobe;
>  
> +extern const dt_provimpl_t *dt_providers[];
> +
>  typedef struct dt_provider {
>  	dt_list_t pv_list;		/* list forward/back pointers */
>  	struct dt_hentry he;		/* htab links */
> @@ -110,6 +115,7 @@ extern dt_provider_t *dt_provider_create(dtrace_hdl_t *, const char *,
>  					 const dt_provimpl_t *,
>  					 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);
>  
>  #ifdef	__cplusplus
>  }
> diff --git a/libdtrace/dt_work.c b/libdtrace/dt_work.c
> index 11335345a..5318cdb7e 100644
> --- a/libdtrace/dt_work.c
> +++ b/libdtrace/dt_work.c
> @@ -362,6 +362,9 @@ dtrace_work(dtrace_hdl_t *dtp, FILE *fp, dtrace_consume_probe_f *pfunc,
>  	dtrace_workstatus_t	rval;
>  	int			gen;
>  
> +	if (dt_provider_discover(dtp) < 0)
> +		return DTRACE_WORKSTATUS_ERROR;
> +
>  	switch (dtrace_status(dtp)) {
>  	case DTRACE_STATUS_EXITED:
>  	case DTRACE_STATUS_STOPPED:
> -- 
> 2.43.5
>