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

[Nick Alcock]

On 21 Oct 2021, Eugene Loh outgrape:
[...]
> 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.

What's irregular about it?

Oh you mean the way they're not all aligned? I never even notice that
(except when the gap is too great, when it damages my ability to read it
enough that I have to delete the extra spacing before I can grok it
properly).

Adjusted, anyway. (The result is harder to read, in my opinion.)

>> 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()?

Er oops! That was not intended at all. ctf_import can fail for a bunch
of reasons :) Fixed! (Also added error-checking to the *other* place
we're calling ctf_import, which was never checking for errors. We now
discard CTF whose importing fails rather than leaving it lying around, a
sliced-in-half ruin, to later torpedo things.)

Have I mentioned how much I hate the C 'check every function tiresomely
for errors but don't have the compiler warn about it' thing? I'm tempted
to add warn_unused_result to a large percentage of libctf's public API,
but that would cause such a storm of warnings from existing users,
*none* of whom check for errors everywhere...

When we can rely on libctf, we can simplify this stuff quite a bit more :)

>> +		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.

Ack.

-- 
NULL && (void)