[DTrace-devel] [PATCH 2/4] Fix comment in SDT provider about the probes we skip

[Kris Van Hees]

On Sat, May 30, 2020 at 04:17:07PM -0700, Eugene Loh wrote:
> On 05/29/2020 09:18 PM, Kris Van Hees wrote:
> 
> > On Fri, May 29, 2020 at 08:10:38PM -0400, eugene.loh at oracle.com wrote:
> >> diff --git a/libdtrace/dt_prov_sdt.c b/libdtrace/dt_prov_sdt.c
> >> @@ -224,10 +224,11 @@ done:
> >>   /*
> >> - * The PROBE_LIST file lists all tracepoints in a <group>:<name> format.  When
> >> - * kprobes are registered on the system, they will appear in this list also as
> >> - * kprobes:<name>.  We need to ignore them because DTrace already accounts for
> >> - * them as FBT probes.
> >> + * The PROBE_LIST file lists all tracepoints in a <group>:<name> format.
> >> + * We need to ignore these groups:
> >> + *   - GROUP_FMT (created by other DTrace processes)
> > This actually refers to kprobes and uprobes created by *any* dtrace process,
> > especially our own because we do not want to register the same probe under two
> > different providers.
> 
> Okay, but the reason it's phrased this way is because it's in 
> populate(), which runs before we create any probes.  So, we simply will 
> not see our own probes.  It seems to me that we can:
> 
> *)  Leave my language as is.
> *)  Add a remark to explain why it is other processes that matter (that 
> is, ours have not yet been created).
> *)  Remove the word "other", thereby using (unnecessarily) general (less 
> informative) language.
> 
> Pick whichever one you want.  I'll do it.

My point is that the comment should document the code.  The test in the code
does not discriminate between this dtrace process and any others, so the
comment shouldn't address a distinction either.

Also consider the fact that you are referring to the fact that this code is in
populate() and that it is used before we create any probes...  and what if that
changes in the future?  Nothing in this code is guaranteeing that no probes
have been created yet, and in fact, it would be perfectly valid to write a
provider that does create probes in the populate phase.  That would render your
comment incorrect - simply because it is adding information that depends on
an assumption - and given that the extra information is not needed, it is
better to leave it out.

> >> + *   - kprobes and uprobes (created by other users)
> > No need to mention "(created by other users)" because even if DTrace created
> > any we would need to ignore them.
> 
> ?  Under what scenario would DTrace create any?  And why would we not 
> explain why we're omitting these?  Isn't this really the same issue as 
> above?

Same as above indeed - so leaving out the "(...)" portion is safer and it
does not change the validity of the comment.  Whether the kprobes and uprobes
are created by other users or not is not relevant - this code needs to ignore
all kprobes and uprobes.

If you want to explain why they are being ignored, you will need to explain
that this provider is exposing tracepoints, and that kprobes and uprobes that
are created using the perf event interface will have events created that show
up in the list of tracepoints under the kprobes and uprobes groups.

I am not sure that this comment needs to go into that much detail though.  We
do not need to defend the need for this - just documenting that we ignore those
two groups would be sufficient here.

> >> + *   - syscalls (handled by a different provider)
> >>    */
> >>   static int populate(dtrace_hdl_t *dtp)
> >>   {
> 
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel