Re: [PATCH] Need -w for destructive actions, even if clause is not used

[Kris Van Hees <kris.van.hees@oracle.com>]

On Fri, Aug 01, 2025 at 02:15:18PM -0400, Eugene Loh wrote:
> On 8/1/25 11:36, Kris Van Hees wrote:
> 
> > On Fri, Jul 11, 2025 at 12:40:24AM -0400, eugene.loh@oracle.com wrote:
> > > From: Eugene Loh <eugene.loh@oracle.com>
> > > 
> > > If a clause includes a destructive action but -w is not used, dtrace
> > > should not start up, even if the clause is ignored (due to -Z).
> > > Solaris treated this as a runtime error.  We should do the same.
> > > 
> > > diff --git a/test/unittest/options/err.no-w-or-destructive2.d b/test/unittest/options/err.no-w-or-destructive2.d
> > > new file mode 100644
> > > @@ -0,0 +1,25 @@
> > > +/*
> > > + * ASSERTION: Without -w or -xdestructive, destructive operations are not ok,
> > > + *            even if a clause will be ignored since it does not exist and
> > > + *            -Z was specified.
> > This could do with some rewriting.
> 
> Sorry, what does "this" refer to?  The comment, the test, or the patch?

The text of the assertion.

> > The issue is that DTrace used to always
> > load all tracing programs, and the kernel would activate them as needed.  We
> > now only load programs for enabled probes (and with -Z active, we may load
> > some programs later).
> > 
> > What we need to be testing here is that if any clauses of a tracing script
> > contain destructive actions, and we are using the script for probing, then
> > an error should be reported unless we are allowing destructive actions (-w
> > or -xdestructive).
> 
> I think we have such a test?

Yes, I just don't think that the way the assertion is phrased well.  E.g.
talking about a clause being ignored since it does not exist.  The clause
very certainly exists (since it is in the script).  It is the probe that
does not exist at the point of initial program construction and loading).

> The case being tested here is where a destructive action is specified, no -w
> is specified, but the clause will not be exercised since the probe does not
> exist and -Z allows us to ignore it.  In this case, should the script be
> accepted or rejected?  The patch says the script should be rejected, which
> was the legacy behavior. (People familiar with the legacy implementation may
> understand why the legacy behavior was what it was, but the question now is
> whether that behavior should be changed.)

Yes, again, it is the way the assertion is phrased that needs fixing I think.
> 
> > > +/* @@runtest-opts: -Z */
> > > +
> > > +bogus:bogus:bogus:bogus
> > > +{
> > > +	system("echo ok");
> > > +}