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

[Kris Van Hees]

On Mon, Jun 15, 2020 at 09:41:10AM -0700, Eugene Loh wrote:
> 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.

Well, yes and no.  You focus on the stability attributes but ignore the bulk
of the defines that are also not uniform.  If we are going to bring uniformity
to these things, lets apply that to all rather than just one thing?

> (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.)
> 
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel