[DTrace-devel] [PATCH] options, test: correct bufsize calculations and tests

[Kris Van Hees]

On Thu, Sep 19, 2024 at 04:39:26PM -0400, Eugene Loh wrote:
> As we have discussed, I do not see the point of such a check.  We change the
> user-supplied value anyhow;  so why should we check it?  How about we come
> to a decision about what to do about this auto resizing stuff (which we've
> changed since legacy DTrace) before fine-tuning what we're checking.  (FWIW,
> I vote for no resizing.  The user typically has no need to specify a size. 
> If they do, we cannot guess if they want their size to be a min, max, or
> what.)

We have discussed this at great length, yes, and since there is no resolution
yet (because it is a more involved problem that needs discussion) and yet we
have failing tests, I think it is best to make sure the tests pass.

We can work on a redesign , but in the meantime, the tests should really pass,
and the current status quo maintained.

> Also...
> 
> On 9/19/24 08:54, Kris Van Hees wrote:
> > Reported-by: Eugene Loh <eugene.loh at oracle.com>
> > Signed-off-by: Kris Van Hees <kris.van.hees at oracle.com>
> > ---
> >   libdtrace/dt_work.c                 |  9 +++------
> >   test/unittest/options/tst.b.d       | 19 ++++++++++---------
> >   test/unittest/options/tst.bufsize.d | 19 ++++++++++---------
> >   3 files changed, 23 insertions(+), 24 deletions(-)
> 
> What about the "too low" tests?

Those pass as expected so no change was needed.

> > diff --git a/libdtrace/dt_work.c b/libdtrace/dt_work.c
> > @@ -268,15 +268,12 @@ dtrace_go(dtrace_hdl_t *dtp, uint_t cflags)
> >   	dtrace_getopt(dtp, "bufsize", &size);
> >   	if (size == 0 ||
> > -	    size < sizeof(struct perf_event_header) + sizeof(uint32_t) +
> > -		   dtp->dt_maxreclen)
> > +	    size < sizeof(struct perf_event_header) + dtp->dt_maxreclen)
> >   		return dt_set_errno(dtp, EDT_BUFTOOSMALL);
> >   	if (dt_pebs_init(dtp, size) == -1)
> >   		return dt_set_errno(dtp, EDT_NOMEM);
> 
> Why does the code special-case "size == 0"?

I honestly cannot remember why that was there.  I bet it was a mistake I made
when I originally wrote that code.  I'll remove it since that is (of course)
meaningless if t is followed by || size < ...