[DTrace-devel] [PATCH 3/7] dtrace: add default options and apply path options earlier

[Kris Van Hees]

Isn't this something that has been a problem even with v1?  Not that we do not
need to fix that - but I am not sure the approach is the best way to fix this,
and if it is not something urgent (i.e. it was even a problem with v1) then
perhaps we can delay this while we brainstorm possible other solutions (e.g.
passing such needed option values to dtrace_open() (and dtrace_vopen()) since
they clearly need it.  Or restructure things so that dt_open becomes a
multi-stage thing?

On Mon, May 15, 2023 at 10:12:19PM +0100, Nick Alcock wrote:
> On 15 May 2023, Kris Van Hees said:
> 
> > Wouldn't it be easier to have dt_open() look for options in environment
> > variables for the things that may need to be tweaked for stuff that is
> > used in dt_open()?
> 
> This is already done, but by cmd/dtrace.c calling dtrace_setoptenv().

That is why I asked about dt_open() doing it which is not cmd/dtrace.c. :)

> After this change, that sets the defaults just like using a command-line
> option would. Before this change, it doesn't apply at the right time,
> and to make them apply at the right time you'd have to scan twice for
> boot-time options (the path sets) and then for everything else, i.e.
> basically all the infrastructure already added in this patch.

> >                     And cmd/dtrace.c could even set such env vars itself
> > based on options passed to it?
> 
> IMHO environment variables are for communicating with subprocesses, not
> yourself. Setting environment variables in yourself (or in anything but
> newly-created subprocesses) is a nonportable, buggy, crash-prone
> nightmare: at *best* it's impossible to use without memory leaks. It's
> best to just pretend that putenv() doesn't exist at all. (It allegedly
> doesn't crash any more, but you need a very recent glibc for that, and
> given its previous history of crashing I frankly don't trust that all
> the bugs are out of it even now.)
> 
> (I actually tried this as one of my failed approaches. It did work but
> it added even more memory leaks, and I'm trying to *eliminate* them, not
> add more -- and these were, by the nature of putenv(), unfixable.)
> 
> > Since there could be consumers that use more than one libdtrace instance,
> > having options that were meant to be per-dtp now becoming global may pose
> > a change in behaviour for libdtrace.
> 
> The caller has two options here -- you can always call dtrace_setopt()
> just as now, on an existing handle, that behaviour is unchanged (it's
> just that a few options won't work that way, but most of them
> historically never did). Alternatively you can always call
> dtrace_setopt(NULL, ...) repeatedly with the same option with no
> negative consequences. (It consumes a bit more memory but does not leak
> it.)
> 
> The only non-horrifying alternative I thought of here was to have *yet
> another ADT* whose only purpose was to track dtrace_setopt()s before the
> handle was created, but having dtrace_open() consume that would change
> its API, it would also require new things just like dtrace_setopt()
> except that they operate on this new ADT rather than a dtrace handle,
> and it's ever so much more complex than what we're doing now.
> 
> OK actually perhaps that is a horrifying alternative.