[DTrace-devel] [PATCH] Implementation of the printa() action

[Eugene Loh]

On 12/07/2020 01:07 PM, Kris Van Hees wrote:

> Given the design for the handling of aggregation data, there is no
> further need for cummulative data retrieval using snapshots.  When we
> are ready to display aggregation data, we can retrieve the data by
> performing a key 0 BPF map lookup on the 'aggs' map.  It gives us all
> data for all CPUs in a single buffer.

I'm trying to review this, but the patch is big and covers a lot of code 
that's unfamiliar to me.  Here are some preliminary comments. They 
aren't meant to be important, but only what I've gotten to so far.

These "new" tests have 2006 (-2020) copyrights.
     test/unittest/actions/printa/err.D_PRINTA_PROTO.fmt_no_arg.d
     test/unittest/actions/printa/err.D_PROTO_LEN.no_arg.d
     test/unittest/aggs/err.D_AGG_REDEF.idx_diff_funcs.d

There is an orphaned string.  Try "git grep idhash_nextoff".

Are these obsolete?
     dtat_ncpus
     dtat_ncpu
     dtat_cpus
dtat_maxcpu seems to be used, but I cannot tell where it is set.  It 
used to be set in aggregate_go().

Should DTRACEOPT_AGGRATE be decommissioned?
How about aggrate (still present in many tests)?
How about dt_lastagg?
How about dtrace_bufdesc and dtrace_bufdesc_t?

Are we changing support for "cpu" (CPU on which to enable tracing)?

In general, I would suggest leaving stylistic refactoring (such as 
eliminating "(void)", changing spacing around "sizeof", use of 
parentheses on "return", etc.) out of big functional patches like this.  
Better to have patches for particular purposes.  If such changes should 
be made, they can be made consistently and as their own patch, 
simplifying review, etc.  That's water under the bridge as far as this 
patch goes, but if you like I can prepare such a patch ("not this week") 
so that there is consistent style in the code base and such refactoring 
will not be part of other patches. Let me know.