[DTrace-devel] [PATCH 13/38] Hide dtrace_actdesc_t until it is needed

[Kris Van Hees]

On Thu, Jul 18, 2024 at 05:06:00PM -0400, Eugene Loh wrote:
> On 7/18/24 16:02, Kris Van Hees wrote:
> 
> > On Thu, Jun 27, 2024 at 01:34:30AM -0400, eugene.loh at oracle.com wrote:
> > > From: Eugene Loh <eugene.loh at oracle.com>
> > > 
> > > Signed-off-by: Eugene Loh <eugene.loh at oracle.com>
> > I think you should just get rid of it.  Worst case, we can always revert the
> > patch.  But this type is only used in code that we do not plan to revive since
> > we no longer do DOF the same way as the older version.  Even if we were to
> > re-implement the functionality to save compiled probe programs to file, it
> > would look very different and certainly not be action-based but rather BPF
> > code based.
> 
> It would seem to me that the declaration (this patch) and the code that uses
> it should go together.  So, are you saying the inoperative code that
> references this declaration should also be removed?  It would seem funny to
> me to remove the declaration (this patch) while (admittedly inoperative)
> code that references it is left alone.

That is a valid point - in which case I think that (for this patch series),
this is not a relevant patch and thus should be dropped.  Instead, a work item
ought to be added to either get rid of (most of) dt_dof.c and related code, or
to actually implement it in a way that makes sense (i.e. do something useful
and be in line with the new implementation of DTrace).

> > 
> > > ---
> > >   include/dtrace/enabling.h | 3 +++
> > >   1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/include/dtrace/enabling.h b/include/dtrace/enabling.h
> > > index f1ec444c..2562c87b 100644
> > > --- a/include/dtrace/enabling.h
> > > +++ b/include/dtrace/enabling.h
> > > @@ -43,6 +43,8 @@ typedef struct dtrace_probedesc {
> > >   	const char	*prb;			/* probe name */
> > >   } dtrace_probedesc_t;
> > > +#ifdef FIXME
> > > +This type is used only in #ifdef FIXME code.
> > >   typedef struct dtrace_actdesc {
> > >   	struct dtrace_difo *dtad_difo;		/* pointer to DIF object */
> > >   	dtrace_actkind_t dtad_kind;		/* kind of action */
> > > @@ -50,6 +52,7 @@ typedef struct dtrace_actdesc {
> > >   	uint64_t dtad_arg;			/* action argument */
> > >   	uint64_t dtad_uarg;			/* user argument */
> > >   } dtrace_actdesc_t;
> > > +#endif
> > >   typedef struct dtrace_ecbdesc {
> > >   	dtrace_probedesc_t dted_probe;		/* probe description */
> > > -- 
> > > 2.18.4
> > >