[DTrace-devel] [PATCH v2 02/11] modules: purge linked-in shared ctf.ko module support

[Eugene Loh]

I'm not qualified to give a solid review of this, but
     Reviewed-by: Eugene Loh <eugene.loh at oracle.com>
Small comments below.

On 10/20/21 2:53 PM, Nick Alcock wrote:
> diff --git a/libdtrace/dt_impl.h b/libdtrace/dt_impl.h
> @@ -167,10 +166,8 @@ typedef struct dt_kern_path {
>   
>   #define DT_DM_LOADED	0x1	/* module symbol and type data is loaded */
>   #define DT_DM_KERNEL	0x2	/* module is associated with a kernel object */
> -#define DT_DM_BUILTIN	0x4	/* module is linked into the core kernel */
> -#define DT_DM_SHARED	0x8	/* module is linked into shared_ctf.ko */
> -#define DT_DM_CTF_ARCHIVED  0x10 /* module found in a CTF archive */
> -#define DT_DM_KERN_UNLOADED 0x20 /* module not loaded into the kernel */
> +#define DT_DM_CTF_ARCHIVED  0x4 /* module found in a CTF archive */
> +#define DT_DM_KERN_UNLOADED 0x8 /* module not loaded into the kernel */
>   
>   typedef struct dt_ahashent {
>   	struct dt_ahashent *dtahe_prev;		/* prev on hash chain */

Maybe this is an opportunity to regularize the use of tabs here?  I 
don't know.  Not important in any case.

> diff --git a/libdtrace/dt_module.c b/libdtrace/dt_module.c
> @@ -582,29 +542,14 @@ dt_module_getctf(dtrace_hdl_t *dtp, dt_module_t *dmp)
>   	ctf_setspecific(dmp->dm_ctfp, dmp);
>   
>   	if ((parent = ctf_parent_name(dmp->dm_ctfp)) != NULL) {
> -		if ((pmp = dt_module_create(dtp, parent)) == NULL ||
> -		    (pfp = dt_module_getctf(dtp, pmp)) == NULL) {
> -			if (pmp == NULL)
> -				dt_set_errno(dtp, EDT_NOMEM);
> -			goto err;
> -		}
> -
> -		if (ctf_import(dmp->dm_ctfp, pfp) == CTF_ERR) {
> -			dtp->dt_ctferr = ctf_errno(dmp->dm_ctfp);
> -			dt_set_errno(dtp, EDT_CTF);
> -			goto err;
> -		}
> +		assert(strcmp(parent, "shared_ctf") == 0);
> +		ctf_import(dmp->dm_ctfp, dtp->dt_shared_ctf);

I don't follow this stuff.  You're saying we no longer need to check the 
return status of ctf_import()?

> @@ -1003,66 +919,19 @@ dt_kern_module_find_ctf(dtrace_hdl_t *dtp, dt_module_t *dmp)
> -		if (strcmp(dmp->dm_name, "shared_ctf") == 0)
> -			/*
> -			 * ctf.ko itself is a special case: it contains no code
> -			 * or types of its own, and is loaded purely to force
> -			 * allocation of a dt_module for it into which we can
> -			 * load the shared types.  We pretend that it is
> -			 * built-in and transform its name to 'shared_ctf', in
> -			 * order to load the CTF data for it from the
> -			 * .ctf.shared_ctf section where the shared types are
> -			 * found, rather than .ctf, which is empty (as it is for
> -			 * all modules that contain no types of their own).
> -			 */
> -			dmp->dm_flags |= DT_DM_BUILTIN & DT_DM_SHARED;
> -		else
> -			strlcpy(dmp->dm_file, dkpp->dkp_path,
> -			    sizeof(dmp->dm_file));
> +		strlcpy(dmp->dm_file, dkpp->dkp_path,
> +		    sizeof(dmp->dm_file));

Cool, but with the reduced indentation, the strlcpy() statement no 
longer needs to continue on a second line.