[DTrace-devel] [PATCH 2/5] slight code refactoring to eliminate gratuitous differences between providers

[Eugene Loh]

On 06/12/2020 06:03 PM, Kris Van Hees wrote:

> On Thu, Jun 11, 2020 at 05:29:38PM -0400, eugene.loh at oracle.com wrote:
>> diff --git a/libdtrace/dt_prov_syscall.c b/libdtrace/dt_prov_syscall.c
>> @@ -40,19 +40,6 @@ static const char		modname[] = "vmlinux";
>>   
>>   #define SYSCALLSFS		EVENTSFS "syscalls/"
>>   
>> -/*
>> - * We need to skip over an extra field: __syscall_nr.
>> - */
>> -#define SKIP_EXTRA_FIELDS	1
>> -
>> -static const dtrace_pattr_t	pattr = {
>> -{ DTRACE_STABILITY_EVOLVING, DTRACE_STABILITY_EVOLVING, DTRACE_CLASS_COMMON },
>> -{ DTRACE_STABILITY_PRIVATE, DTRACE_STABILITY_PRIVATE, DTRACE_CLASS_UNKNOWN },
>> -{ DTRACE_STABILITY_PRIVATE, DTRACE_STABILITY_PRIVATE, DTRACE_CLASS_ISA },
>> -{ DTRACE_STABILITY_EVOLVING, DTRACE_STABILITY_EVOLVING, DTRACE_CLASS_COMMON },
>> -{ DTRACE_STABILITY_PRIVATE, DTRACE_STABILITY_PRIVATE, DTRACE_CLASS_ISA },
>> -};
>> -
>>   struct syscall_data {
>>   	dt_pt_regs	*regs;
>>   	long		syscall_nr;
>> @@ -67,6 +54,19 @@ struct syscall_data {
>>   #define ENTRY_PREFIX	"sys_enter_"
>>   #define EXIT_PREFIX	"sys_exit_"
>>   
>> +/*
>> + * We need to skip over an extra field: __syscall_nr.
>> + */
>> +#define SKIP_EXTRA_FIELDS	1
>> +
>> +static const dtrace_pattr_t	pattr = {
>> +{ DTRACE_STABILITY_EVOLVING, DTRACE_STABILITY_EVOLVING, DTRACE_CLASS_COMMON },
>> +{ DTRACE_STABILITY_PRIVATE, DTRACE_STABILITY_PRIVATE, DTRACE_CLASS_UNKNOWN },
>> +{ DTRACE_STABILITY_PRIVATE, DTRACE_STABILITY_PRIVATE, DTRACE_CLASS_ISA },
>> +{ DTRACE_STABILITY_EVOLVING, DTRACE_STABILITY_EVOLVING, DTRACE_CLASS_COMMON },
>> +{ DTRACE_STABILITY_PRIVATE, DTRACE_STABILITY_PRIVATE, DTRACE_CLASS_ISA },
>> +};
>> +
>>   /* Scan the PROBE_LIST file and add probes for any syscalls events. */
>>   static int populate(dtrace_hdl_t *dtp)
>>   {
> I do not understand the rationale for the change.  I can think of some
> re-ordering that could be done, but not what you propose here,  Cam you
> explain why you chose to to this?

Sure.  There is a pattern of overall organization that is evident in the 
various dt_prov_*.c files, with one exception being the placement of the 
stability attributes in this one file.  There is no justification (that 
I know of) for that one egregious exception; meanwhile, uniform 
organization would be nice for developers.

Incidentally, here is that overall organization:

     +---------------  dt_prov_dtrace.c
     | +-------------  dt_prov_fbt.c
     | | +-----------  dt_prov_profile.c
     | | | +---------  dt_prov_sdt.c
     | | | | +-------  dt_prov_syscall.c
     | | | | |
     | | | | |
     v v v v v

     * * * * *  Copyright notice and introductory comments
     * * * * *  include files
     * * * * *  define names like prvname, modname, funname
             *  STABILITY ATTRIBUTES
     * * * * *  define preprocessor symbols (tracefs files, prefixes, 
suffixes)
     * * * *    STABILTIY ATTRIBUTES
     * * * * *  populate()
     * * * * *  trampoline()
     * * * * *  probe_info()
         *      provide()
     * *        probe_fini()
     * * * * *  dt_provimpl_t = { }

You can see that moving the stability attributes in that one file 
improves the uniformity.

(Not all providers have provide() or probe_fini() functions.  Also, the 
inclusion of the profile provider is a little anachronistic -- it 
appeared in a later commit -- so you can omit that column if you like.)